fix(gfql): pre-existing engine-parity/correctness bugs (ORDER BY DESC nulls, predicate from_json, str value-safety)#1732
Merged
Merged
Conversation
F2 from the OLAP-stack review. The general row pipeline hardcoded nulls-last for every ORDER BY key on all engines, so `ORDER BY <col> DESC` over a column with NULLs was mis-ordered and `DESC ... LIMIT k` silently returned the wrong top-k (dropping the NULL group, which openCypher ranks first since NULL is largest). - pandas/cuDF (row/pipeline.py): per-key null-indicator single-pass sort (ind = col.isna(), sorted in the key's own direction) — stability-free so cuDF's non-stable quicksort is safe; scalar na_position cannot be per-key. - polars (order_by_polars): per-key nulls_last list aligned with exprs. - test_order_by_null_placement.py: hand-derived openCypher oracle, ASC+DESC x grouped+scalar, across pandas/polars/cuDF. Mutation-verified vs pristine master (wrong [C,B] -> correct [None,C]) on all three engines. Pre-existing on master (not introduced by the OLAP split); the OLAP fast paths already ordered nulls correctly, so this aligns the general/fallback path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
Bug #1 from the OLAP-stack review. The predicate from_json registry mapped EQ/NE/GT/GE/LT/LE/Between/IsNA/NotNA to predicates.numeric (numeric-only), but Cypher lowering and the public predicate API build predicates.comparison (which also accept strings + temporals). Same __name__ meant a JSON round-trip downgraded a live comparison predicate to numeric-only, raising GFQLTypeError "val must be numeric" for a string/temporal equality or a temporal comparison (breaks remote GFQL, serialization, caching). Fix: from_json imports the comparison versions. Numeric-valued predicates are unaffected (comparison is a superset). Round-trip regression tests added to test_from_json.py (string/temporal/numeric eq + temporal gt), mutation-verified (revert import -> 10 tests fail). Full predicate suite 139 passed/0 failed on GPU. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
Bug #2 from the OLAP-stack review. Contains/Startswith/Endswith/Match/Fullmatch called `s.str.<op>` directly, raising an opaque AttributeError ("Can only use .str accessor with string values!") on a numeric/temporal/bool column (pandas and cuDF). Fix: guard each predicate — a non-string-capable series yields an all-null result (openCypher: string op over a non-string is null -> excluded), mirroring the per-cell behavior on an object column holding non-strings. Never stringifies (would diverge pandas<->cuDF, e.g. wrongly match `5 CONTAINS '5'`). String, mixed-object, and all-null columns unchanged. Helpers _series_supports_str_ops / _nonstring_null_result. Regression tests in test_str.py across dtypes; cuDF parity confirmed (int -> all-null, not stringified). str suite 106 passed. Scope: the 5 pattern predicates. The str-property _CallablePredicate family (IsNumeric/IsAlpha/...) is deferred — its semantics on non-string dtypes are genuinely ambiguous, tracked for a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
…n non-string column Bugs #3 (polars connected-join clean-NIE) + #4 (pushdown polars-dtype) from the OLAP-stack review. A string predicate (CONTAINS/STARTS WITH/ENDS WITH/regex) on a Categorical/Enum/numeric column raised a clean GFQLSchemaError on pandas/cuDF but leaked polars' opaque InvalidOperationError ("expected String type, got: cat"). filter_by_dict_polars now raises the same GFQLSchemaError (E302) so all three engines agree; categorical is treated as non-string exactly as filter_by_dict. Investigation (documented): the connected-join predicate-pushdown dtype gate is parity-correct on polars for string/numeric/eq/bool columns (pushdown_probe: PARITY=True on all) — no silent-wrong push found; the only divergence was this opaque-vs-clean error, now fixed. Regression test test_polars_string_predicate_nonstring.py; polars conformance 406 passed/0 failed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
Bug #6 (R4) from the OLAP-stack review. pandas coerces CASE branches to a common type, but cuDF's `.where` raises TypeError "cudf does not support mixed types", surfaced as a hard GFQLTypeError. Bit `CASE WHEN path IS NULL THEN -1 ELSE length(path) END` over an UNREACHABLE shortestPath (int -1 vs object/null hops branch): pandas returned -1, cuDF errored. Fix: the row-AST CASE evaluator (_gfql_eval_expr_ast) now catches the cuDF mixed-type TypeError, unifies the branches via _gfql_coerce_case_branch_dtypes (common numeric dtype, then string fallback), and retries -> cuDF returns the same value pandas does. Reachable paths keep clean int parity (direct .where succeeds, no coercion). Validated on #1705 (shortestPath): reachable 0->2 = 2 both engines; unreachable 0->4 = -1 (pandas) / -1.0 (cuDF, value-equal, no raise). General regression test test_case_mixed_dtype.py (all-null object column, same mechanism, no shortestPath) mutation-verified: pristine master cuDF RAISES, fixed cuDF returns -1. CASE test suite 58 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
The new order-by-null and polars-string-predicate regression tests hardcoded engine="polars", which ImportErrors on the CPU CI lanes (test-gfql-core / test-core-python) that ship without polars. Guard polars like cudf (_HAS_POLARS), so those lanes run pandas(+cudf) and the test-polars lane runs polars. No behavior change where polars is installed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
The changed-line-coverage gate (combines core + gfql + polars lanes; NO cuDF lane) flagged the cuDF-only branches and two polars lines. - cuDF-only branches marked `# pragma: no cover` with rationale (structurally uncoverable by this gate — no cuDF lane; all validated on dgx + mutation- verified): str.py _nonstring_null_result cuDF branch, pipeline.py CASE mixed-dtype except + _gfql_coerce_case_branch_dtypes helper. - order_by_polars: restructured the scalar-vs-list `descending` branch into a single coverable path (normalize to a per-key list); multi-key ORDER BY validated parity pandas/polars on dgx. - Added the new polars regression tests (test_polars_string_predicate_nonstring, test_order_by_null_placement) to bin/test-polars.sh so the polars lane actually exercises (and covers) the GFQLSchemaError raise + null-order paths. No production behavior change beyond the order_by_polars refactor (parity- validated). 24 order-by/string-predicate tests pass incl. polars. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov
marked this pull request as ready for review
July 19, 2026 06:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three PRE-EXISTING master bugs surfaced during the GFQL OLAP-stack review + test-amplification (not introduced by #1729–#1731 / #1705). Held as draft — no merge, no force-push. Validated across pandas/polars/cuDF on dgx-spark.
Fixes
ORDER BY ... DESCNULL placement (HIGH, silent-wrong) — openCypher orders NULL as largest (DESC→ nulls first), but the general row pipeline hardcoded nulls-last for every key, soDESC ... LIMIT ksilently dropped the NULL group. Fixed pandas/cuDF (per-key null-indicator single-pass; cuDF non-stable-sort-safe) + polars (nulls_lastlist). Mutation-verified vs pristine master ([C,B]→[None,C]) on all 3 engines.from_jsondowngraded comparison→numeric —EQ/NE/GT/GE/LT/LE/Betweendeserialized as numeric-only, so a string/temporal=or temporal< >round-trip raisedGFQLTypeError "val must be numeric". Now registers thepredicates.comparison(numeric+temporal+string) versions.Contains/Startswith/Endswith/Match/FullmatchraisedAttributeErroron numeric/temporal/bool columns; now openCypher-null (excluded), never stringifying (parity-safe).Validation (dgx-spark, 26.02-gfql-polars)
test_order_by_null_placement.py(12),test_from_json.pyround-trips,test_str.pyvalue-safety.🤖 Generated with Claude Code