feat(gfql): plan connected join predicate pushdown#1729
Draft
lmeyerov wants to merge 36 commits into
Draft
Conversation
…t-2-connected-plan # Conflicts: # graphistry/compute/gfql/cypher/lowering.py
lmeyerov
changed the base branch from
perf/gfql-olap-split-1-foundation
to
master
July 16, 2026 15:37
…ry literal branch The py3.12 per-file coverage baseline gate failed on lowering.py (88.93% vs 89.05% floor). Two changes address it: Add a mixed connected-comma test exercising unary pushdown alongside an IN residual, covering a real planner path that had no coverage. Remove the UnaryOp branch of _connected_join_expr_literal_value. It is unreachable: the grammar folds signed numeric literals, so `p.age >= -1` parses to Literal(-1) and a surviving UnaryOp always wraps a non-literal operand (`-x` -> operand=Identifier). Param substitution rewrites text before parsing, so `-$minAge` folds the same way or fails closed on `--2`. The two callers both consume _parse_row_expr output via BinaryOp.left/.right, and the UnaryOp import stays live for six other uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The dead-code removal alone left lowering.py at 89.0022%, passing the per-file floor only by exact equality with the 0.05 tolerance, and left changed-line coverage at 76.06% on the gfql lane, under the 80% gate. The connected-join planner pushes LabelRef predicates onto pattern targets, a real branch of this PR's new code that had no coverage. Cover it with a labelled T1 fixture and a label-plus-property query, asserting both the count semantics and that label__Person and age reach the pushed filters. lowering.py now clears its floor outright at 89.14% (no tolerance needed) and changed-line coverage reaches 80.28% on the gfql lane alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Commit aa06536 deleted the UnaryOp branch of _connected_join_expr_literal_value claiming it was unreachable. That was wrong. NUMBER carries its sign as a lexer terminal (/[+-]?.../), so a signed literal only folds when the sign is lexically adjacent: `-1` folds to Literal(-1), but `- 1` and `-(1)` keep the UnaryOp node and reach the helper with an ExprLiteral operand. The earlier probe only tried `-1` and `-x` and over-generalised from them. Removing the branch silently stopped pushing those comparisons onto pattern targets, degrading them to a where_rows residual. Results stayed correct, so no test caught it -- an optimization regression, not a wrong answer. Restore the branch and cover it with a parametrized `- 26` / `-(26)` / `+(26)` test asserting the filter reaches the pushed targets. Removing the branch again fails all three. Rename the mixed test, whose `-1` folds and never exercised a unary node, to say what it actually covers. lowering.py clears its per-file floor outright at 89.16% (3149/3532) with the branch restored, so the coverage gate needed real tests, not the deletion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Connected-join pushdown moved WHERE atoms onto pattern targets without checking the op/literal pairing was representable there, regressing two shapes against master: WHERE p.nick = $v (v=None) master [] -> HEAD n=4 (every row, unfiltered) WHERE p.nick <> 'a' master n=2 -> HEAD GFQLTypeError Null equality: the executor re-binds the pushed chain through ASTNode.to_json, and _filter_dict_to_json drops null-valued entries, so the predicate silently disappeared and the residual that would have applied it had already been dropped. Non-numeric ordering: !=/</<=/>/>= lower to NumericASTPredicate, which admits only int/float, so a string or bool literal raised where master answered from a where_rows residual. Gate pushdown on representability and fall back to the residual otherwise. Parameters are resolved before the check -- $v arrives as an unresolved ParameterRef, and judging the ref rather than its value is how the null case got through. Unresolvable refs stay residual so the missing-parameter error still surfaces from its normal path. Pushdown is unaffected where it was already valid: numeric compares, string equality, and unary literals still push with no residual. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
… dropping identity Three more regressions this PR introduced against master, all found by independent adversarial review and verified against master 770d2a5: p.age = $v {"v":{"type":"GT","val":26}} master [] -> n=2 (ran `age > 26`) count(DISTINCT p), node id column != 'id' master 3 -> 1 p.nick STARTS WITH 'a' pre-guard 1 -> error (my guard over-rejected) Predicate injection: _filter_dict_to_json passes a raw dict through verbatim and maybe_filter_dict_from_json revives it via predicates_from_json, so a caller-supplied map param executed as an arbitrary predicate. Gate `==` to scalars. count(DISTINCT): the count() early-return left a bare alias unrewritten. It never checked FunctionCall.distinct, and a bare alias only resolves when the node id column is named `id`/`p`; otherwise it degrades to a constant and collapses the count. Master rewrites to the identity column, so drop the early-return and let it. String predicates: contains/starts_with/ends_with/regex lower to real round-trippable ASTPredicates and the residual cannot render them at all, so rejecting them turned working queries into errors. Allow them to push. Tests use a real Plottable, not _CypherTestGraph: the executor's serialization round-trip is where the dropped-null and revived-predicate filters bite, and the test double cannot observe it -- which is why the PR's own tests missed all of this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
A pushed filter_dict is schema-validated against the real column, while the row residual evaluates leniently (Cypher 3-valued logic yields no rows). Pushing a type-incompatible atom therefore turned a correct empty result into a hard error: p.age = 'foo' master [] -> GFQLSchemaError (string literal, numeric column) p.nick >= 26 master [] -> GFQLTypeError (numeric literal, string column) No value-type rule can fix this, because safety depends on the column's dtype and not the literal's. Compilation had no way to know: compile_cypher() takes no graph, so `p.age = 'foo'` and `p.nick = 'a'` are indistinguishable at lowering. Thread the node dtypes gfql() already has down to the pushdown decision, and admit an atom only when the column accepts it (mirroring compute/validate_schema.py). Without dtypes -- bare compile_cypher(), which never executes -- the value-type decision stands, so translation-only callers are unaffected. The planner now picks per atom: numeric compares, string equality, string predicates, and unary literals push with no residual; dtype-incompatible atoms fall back to the residual and answer correctly. Every probed shape now matches master, except STARTS WITH/CONTAINS/=~, which this PR makes work where master errors. Also set lowering_py_max_lines to the exact count rather than leaving headroom a later rung could absorb silently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Independent review of bfde19b found the gate mirrored the wrong file and left three divergences from master: p.flag = 'yes' (bool column) master [] -> GFQLSchemaError p.flag = 99999999999999999999 master range error -> raw OverflowError p.age <> 99999999999999999999 master range error -> n=4 compute/validate_schema.py is dead code -- only a docs test imports it -- and it disagrees with the validator that actually runs, compute/filter_by_dict.py, on bool: the live one counts bool as numeric. The gate's `and not is_bool_dtype(...)` carve-out therefore admitted a string against a bool column, straight into the error the gate exists to prevent. Reuse the live validator's own _is_numeric_dtype_safe / _is_string_dtype_safe so the two agree by construction and stay correct on non-pandas dtypes. The 64-bit literal guard lives on the row-expr path, so pushing an out-of-range int evaded it and reached pandas. Reject those and let the residual report the range error. _node_dtypes_for_pushdown used dtypes.items(), which only pandas and cuDF expose; polars' .dtypes is a plain list, so it raised, was swallowed, and returned None -- silently reverting every polars-backed graph to the value-type-only decision. Zip columns with dtypes, which is correct on all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…olars The dtype gate was inert on polars-typed nodes, so the exact bug it was added for came back on them: polars nodes, WHERE p.age = 'foo' master [] -> GFQLSchemaError pd.api.types.is_numeric_dtype(pl.Int64()) returns False rather than raising, so filter_by_dict's helpers never reach the kind/text fallback that exists to handle non-pandas dtypes. Every polars column came back neither-numeric-nor-string, and `isinstance(value, str) -> return not is_numeric_col` read that as pushable. The gate's dtypes came from the polars frame while the validator saw the pandas-converted one. Classify explicitly: use the helpers, and when both say False re-derive from kind/text. Unrecognized dtypes now fail closed rather than being read as safe. Polars verdicts are now identical to pandas, so ordering pushdown -- previously rejected outright on polars, making the optimization dead there -- engages as intended. Tests cover the fallback with synthetic dtype objects so the pandas lane exercises it (the gfql lane has no polars and would skip), plus real polars cases via importorskip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The dtype text fallback matched substrings, and container dtypes embed their element type, so it read them as scalars: interval[int64, right] contains "int" -> numeric List(Int64) contains "int" -> numeric struct contains "str" -> string An IntervalDtype node column then pushed a comparison and crashed where master answers: WHERE p.iv > 1 master [] -> raw builtins.ValueError WHERE p.iv <> 1 master n=8 -> GFQLTypeError The underlying pandas ValueError is pre-existing -- g.chain() with GT on an interval column raises identically on master -- but pushdown made it reachable from Cypher, where master went through the residual and answered. It also leaked unwrapped, while master wraps it as a GFQL error. Test container tokens before the scalar ones so these fail closed. Exposure was low -- pd.cut yields category dtype, which already failed closed -- but the crash direction was the gate's stated contract inverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
A connected comma join with a string-valued inline property map and a pushable WHERE atom
on that same property hard-errored on a query master answers:
MATCH (person:Person {id:'p1'})-[:L]->(city:Place),
(friend:Person {nick:'aa'})-[:L]->(city)
WHERE friend.nick = 'aa' RETURN count(friend) AS n
master [{'n': 1}] -> GFQLTypeError: val must be numeric (int or float) | value: 'str'
Pushing merges with the inline value via _merge_filter_predicates, which wraps raw scalars
using comparison.eq. That serializes to {'type': 'EQ'}, but predicates/from_json.py binds
the EQ tag to predicates.numeric.EQ, which admits only int/float -- so the executor raises
when it rehydrates the string.
That write/read registry split is pre-existing and lives well outside this PR; master never
reached it because it never pushed connected-join WHERE atoms, leaving inline values as raw
scalars that serialize without an EQ wrapper. Fixing the split belongs in its own change.
So don't create the shape: skip pushdown when an inline map already set a non-numeric scalar
on the property, and let the residual answer as before. Absent properties, numeric inline
values, and already-built predicates are unaffected and still push.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The merge guard inspected only the existing value, so it was order-dependent. filter_dict is mutated as earlier atoms in the same WHERE push, and a previously-pushed string predicate is an ASTPredicate -- which the guard green-lit -- so the next raw string merged behind it into the same broken EQ shape: WHERE p.name CONTAINS 'al' AND p.name = 'alice' -> GFQLTypeError: val must be numeric WHERE p.name = 'alice' AND p.name CONTAINS 'al' -> blocked correctly The error named no numeric anywhere in the query, and CONTAINS works standalone on this branch, so the shape is one a user would reasonably write. Check the incoming value too: `_predicate_value` keeps `==` as a raw value while every other op builds a real predicate that round-trips on its own tag, so refuse the merge when `==` carries a non-numeric. Both orders are now safe, and the first answers correctly (n=4) where master could not render the clause at all. Numeric merges still push. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…terializes
The gate classifies the dtype the planner sees, but filter_by_dict validates the frame the
executor materializes, and those disagree for polars Decimal -- numeric to the planner
("decimal" token), object to pandas, which the validator calls string:
polars Decimal column, WHERE p.age > 25 master n=8 -> GFQLTypeError E302
polars Decimal column, WHERE p.age = 26 master n=4 -> GFQLSchemaError E302
Master answered these correctly because it never pushed. Categorical/Enum split the same way
(polars Categorical -> pandas category, which classifies as neither).
Drop decimal/categorical/enum from the token lists so they fall through to fail closed. The
answer stays correct via the residual and only pushdown is lost -- the same trade pandas
`category` already makes. The robust fix is to read dtypes after materialization so planner
and validator judge the same frame; that is a larger change than this rung.
Adds the first end-to-end polars test that actually pushes: every prior polars assertion was
negative (asserting a rejected predicate), which is why this went unseen.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Arrow-backed columns raised on queries master answers: ArrowDtype(dictionary(int32,int64)), WHERE p.year > 2000 master n=2 -> GFQLTypeError E302 ArrowDtype(bool), WHERE p.flag >= 1 master n=2 -> GFQLTypeError E302 Reachable from pd.read_parquet(dtype_backend='pyarrow') and any dictionary-encoded column. Two independent causes, and the earlier token patches only papered over the second: The gate fell back to kind/text whenever both filter_by_dict helpers returned False, but those helpers fall back only on `except`. pd.api.types.is_numeric_dtype returns False without raising both for a dtype pandas cannot parse and for one it authoritatively knows is not numeric, so the trigger cannot tell those apart -- and overriding an authoritative False is what raised here, since the executor re-applies it. With pandas nodes df_to_engine is a no-op, so planner and executor held the identical dtype object and still disagreed. Drop the fallback and defer to the helpers, so the two agree by construction. That alone would lose all pushdown on polars, whose dtypes satisfy neither helper. So classify the frame the executor materializes rather than the one the caller passed: df_to_engine(nodes.head(0), ...) preserves dtype classes, and polars Int64 -> int64 pushes while Decimal -> object correctly does not. This subsumes the decimal/categorical/enum and container token lists, which are removed: polars List materializes to object, so an ordering push is declined without a special case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
… without head() pyarrow.Table nodes are supported and master executes against them, but pa.Table has no .head(), so the dtype probe raised, the bare except swallowed it, and the gate returned None. None means "no graph, use value-type rules", so it pushed dtype-blind -- the exact bug class the gate exists to prevent: arrow nodes, WHERE p.name = 5 master [] -> GFQLSchemaError Two conflations, both fixed: None answered both "there is no graph" and "we could not read the graph we have". The first is safe: a bare compile_cypher() has no dtypes and value-type rules apply. The second must fail closed. Now only a missing _nodes returns None; a schema we failed to read returns an empty mapping, which fails every column lookup and falls back to the residual. `return mapping or None` also turned an empty read into the unsafe answer. The probe assumed .head(). Fall back to the frame itself when absent -- df_to_engine converts a pa.Table to pandas, so the gate now reads real dtypes for arrow nodes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
polars -> pandas conversion is data-dependent, so an empty probe reports a different class than the real thing: a nullable Boolean is `bool` at head(0) and `object` once converted. The gate called it numeric while the executor filtered an object column: polars nodes, nullable bool, WHERE p.flag > 0 master n=4 -> GFQLTypeError The claim that head(0) preserves dtype classes was wrong. Nullable Int64 widens int64 -> float64, which stays numeric and is harmless, but Boolean crosses the class boundary. Convert the whole frame instead of guessing which conversions are data-dependent. For pandas nodes df_to_engine is identity and costs nothing; for others it is one conversion the executor performs anyway. No heuristic to get wrong. The old test asserted probe dtypes in isolation and would pass with this bug present. It is replaced by one that compares against the full conversion -- the property that actually failed -- and asserts the empty probe disagrees. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Reading dtypes converts the whole nodes frame, and the gate was evaluated eagerly as a call argument for every string gfql(). Only connected-join pushdown consumes it, and that path needs an aggregate RETURN, so most queries paid for a frame that was immediately discarded: 2M-row polars nodes, MATCH (n) WHERE n.age > 900 RETURN n 187ms -> 307ms (+64%) The conversion is linear (~65ms per million polars rows) and hits every query on a polars-backed lane. pandas is unaffected -- df_to_engine is identity there. The comment claiming it was "one conversion the executor performs anyway" was wrong: the probe's frame is discarded and the executor converts again, so it was 2x, not free. Return a lazy Mapping that materializes on first lookup and caches. Callers are unchanged, queries that never reach pushdown pay ~0.01ms, and the full-frame read -- which is what makes the dtype classes correct -- still happens whenever a decision actually needs it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Reverting the empty-mapping fallback to None survived all 1275 tests: the two are observationally equivalent end-to-end, since a frame whose schema cannot be read also fails execution the same way either way. But the distinction is load-bearing -- None means "no graph, use value-type rules" and pushes dtype-blind -- so assert it directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Numeric comparisons against a pandas bool column raised on queries master answers: MATCH (i)-->(p), (p)-->(c) WHERE p.flag > 0 master n=2 -> GFQLTypeError (same for >=, <>, and = 1; only = true survived) The gate reads the source nodes frame, but the connected-join executor filters a joined one, and an unmatched row introduces NaN. bool cannot hold NaN, so pandas widens it to object -- numeric to the gate, string to the validator. So "classify the frame the executor filters" was still not what the code did: it classified the frame before the join. bool is the only common dtype whose widening crosses the class boundary; int64 -> float64 stays numeric and keeps pushing. Decline bool rather than model the join's type algebra -- the residual answers, and guessing at conversions is what produced most of the defects here. The single-pattern path fails the same way on master, so the defect is pre-existing; this join shape was immune only because it never pushed. Existing bool tests covered equality only, and the polars nullable-Boolean test passes because that dtype converts to object and is declined -- neither exercised a pushed numeric op on a native bool. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
A bool column with a null is object, and string predicates are admitted on dtype alone -- by this gate and by filter_by_dict alike -- so .str failed on the values and leaked a raw exception where master raised a GFQL error: flag=[True,False,None,True], WHERE p.flag CONTAINS 'a' master GFQLValidationError -> builtins.AttributeError: Can only use .str accessor... Both checks are dtype-level while .str fails at the value level, so "planner and validator agree by construction" held and still failed. Declining `object` is not the fix: pandas stores ordinary strings there, so that would delete CONTAINS/STARTS WITH entirely. Instead omit object columns whose infer_dtype is a homogeneous non-string kind. An absent column has nothing to look up, so the gate fails closed and the residual answers, as it did before. Mixed and empty contents are untouched -- they already work. Real string columns keep pushing: p.name CONTAINS 'o' still answers where master cannot render the clause at all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The omit-set listed kinds to reject and missed three that .str also rejects, so they still
leaked a raw AttributeError where master raised a GFQL error:
score = [1.5, 2.5, None, 4.5] (mixed-integer-float) -> builtins.AttributeError
clock = [time(1), time(2), None, time(3)] (time) -> builtins.AttributeError
object column of pd.Interval (interval) -> builtins.AttributeError
mixed-integer-float is the sharp one: it is what a float column with a None infers as, while
np.nan infers as floating. So the previous commit caught "a bool column that acquired a null"
and missed the float column that acquired one -- its own motivating case, one dtype over.
StringMethods._validate admits exactly {string, empty, bytes, mixed, mixed-integer}. Keep
those and omit the rest, minus bytes, which str.contains forbids. Mirroring the rule fixes
all three and fails closed on kinds pandas adds later; an enumerated denylist can do neither.
Also repairs the totality premise of the full-conversion test, which the previous commit
broke: the map is partial by design now, and the test asserted every column was present. It
failed only under system python3 -- the polars lane skips it in CI, so 77/77 stayed green
with a broken test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Adding "bytes" back to the allowlist survived all 1292 tests, yet it is load-bearing: without the carve-out, an object column of bytes leaks a raw TypeError where master raises a GFQL error. The subtlety is two-layer. bytes passes StringMethods._validate, so mirroring that allowlist verbatim looks right -- but the methods themselves reject it via @forbid_nonstring_types, so str.contains still fails. Nothing pinned the difference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Admitting "floating" or "integer" back into the keep-set survives all 1293 tests, yet each reintroduces the raw AttributeError the omit-set exists to prevent -- an object column of plain floats or ints is the most trivially constructible case of it. The existing test parametrized only mixed-integer-float, time, and boolean; bytes got its own test. The two simplest kinds were pinned by nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
mixed and mixed-integer passed pandas' accessor rule on the source frame, but the pushed filter runs on the join's candidate subset -- and those kinds are not closed under it. Drop the strings and they collapse to integer/floating/boolean, which .str rejects: value=["alpha",10,20,30,"beta"], p-candidates hold only numbers WHERE p.value CONTAINS 'a' -> builtins.AttributeError (raw) Master never raises there, but only because it never applies the filter -- its n=2 is wrong. This is the third defect from one root: the gate reasons about a frame the executor does not filter. Pre-conversion dtypes, then post-join widening, now post-subset content. So admit only what no subset can invalidate: string and empty. A subset of strings is string or empty, both accepted. Mixed columns now decline to a typed error, matching master, which cannot render CONTAINS at all. Pure string columns keep pushing and answering where master rejects them. The durable fix is to make the pushed predicate itself value-safe, which would also repair master's single-pattern leak; that reaches into shared predicates and belongs in its own change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Dropping "empty" from the keep-set discriminates zero tests across the whole 4,530-test gfql suite, yet it is load-bearing: an all-null object column infers `empty`, and `.str` handles it, so CONTAINS correctly returns [] (3VL: NULL CONTAINS 'a' is NULL, no rows). Drop it and those queries silently revert to master's unsupported error. Its sibling "string" causes 15 failures when dropped -- the asymmetry is what exposed the gap. "empty" is also load-bearing for the subset-closure argument itself: a subset of strings is string or empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
… pattern A pushed filter that emptied a pattern returned a schema-less frame and skipped post_join_chain -- where count(p) AS n lives -- so the RETURN column vanished: WHERE p.s_col = 'zzz' (string, pushed) cols=[] <- master: cols=['n'] WHERE p.m_col = 'zzz' (mixed, not pushed) cols=['n'] Same query shape, same emptiness; the only discriminator was whether the dtype gate admitted the column, so consumers doing df["n"] hit a KeyError on one and not the other. Both early-returns are byte-identical on master. The regression is reachability: master never pushed into the pattern, so the filter ran post-join and the aggregate survived. This is the fourth defect from that one root -- the pushdown reaching code paths master never exercised. An emptied pattern still carries its columns, so let it flow: the joins propagate empty-with-schema and post_join_chain runs on empty input, which is what produces the column. Projections were never affected; they don't take this path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…mpties a pattern" This reverts commit 77df4f7. Its central claim -- "an emptied pattern still carries its columns, so let it flow" -- is false for edge aliases, and I shipped it without checking. The `rows` binding-op emits no edge-alias payload at 0 rows, so `e1.w` and the `e1.e1` marker vanish. `joined_alias_columns` recovers node aliases via its `suffix == "id"` fallback -- which is why count(p)/count(q) passed and the claim looked true -- but edge aliases have no `.id` and are only recoverable from the marker the emptied pattern never emits. post_join_chain then dereferences a column that isn't there: MATCH (p)-[e1]->(q), (p)-[e2]->(r) WHERE p.i_col = 999 RETURN count(e1) AS n master cols=['n'] rows=0 -> GFQLTypeError So the commit made the very thing it set out to preserve strictly worse: master returns the ['n'] schema, and it raised. A MEDIUM schema drop was traded for a HIGH failure on valid queries. Reverting restores the prior behaviour, which reinstates the original schema drop (#25). That defect stands, recorded, with its real fix: make the `rows` binding-op emit the full binding schema at 0 rows, so the invariant is established rather than assumed. Restoring the early-return is not that fix -- it just stops making things worse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The except path returned False, keeping the column -- the opposite of what its own docstring
claimed and of what its sibling _read_node_dtypes does (returns {} for a schema it cannot
read). An object column we cannot inspect tells us nothing about whether .str would reject
its values, so keeping it pushes blind.
Split the two failure modes: a dtype check that raises means the column isn't ours to judge
(keep); an unreadable object column means omit and let the residual answer.
Latent rather than live -- the reachable paths (duplicate column labels, non-str labels) are
already outside the supported envelope or unreachable through the parser. Fixed because "fail
closed on what you cannot verify" is the rule the rest of this gate follows.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…through #25 root fix. A pushed filter that emptied a connected-join pattern returned a schema-less frame and skipped post_join_chain, dropping the aggregate's RETURN column. #27 showed why removing the early-returns alone (77df4f7, reverted) is wrong: the `rows` binding-op drops edge-alias columns at 0 rows -- `_gfql_connected_bindings_state` assembles the schema incrementally, so an emptied hop returns before adding edge-alias and later-node columns. Node aliases survive via the row-frame merge's empty lookup; edge aliases have no `.id` fallback, so count(e1) on an emptied pattern raised. Fix the root: at the end of the bindings row-frame build, when 0 rows, add the declared alias.alias markers and edge-alias columns (derivable from ops + the base edge frame). That makes the emptied frame carry its full schema -- instrumented: the rows op now emits all 13 columns at 0 rows, matching the non-empty shape. Only then remove the two early-returns in _apply_connected_match_join so the empty-with-schema frame flows through post_join_chain. Verified vs master: count(e1)/count(DISTINCT e1)/edge-in-WHERE on an emptied pattern, plus the #25 node case, all match master (cols=['n'], rows=0); non-empty controls unchanged; OPTIONAL MATCH (shares the rows op) and polars unchanged. Both halves mutation-verified. The 0-row bare aggregate still returns rows=0 where Cypher wants one row n=0 -- pre-existing on master, unchanged here, recorded as a separate latent finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
The #25 schema fill used an untyped None broadcast, so an emptied edge-alias column came back object where the non-empty path and master give int64: WHERE a.v > 99999 RETURN sum(e1.w) master int64 [] -> object [] Values were always correct, but the object dtype upcast the aggregate and escaped a 0-row branch into a non-empty result via UNION ALL (the concatenated column became object). Fill from a typed empty slice of the source edge frame instead, mirroring the node path's typed lookup; the alias.alias marker has no source dtype and keeps the None broadcast. sum/avg/max/count over an emptied edge-alias pattern, and the UNION ALL escape, now match master's dtype exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Companion to the edge-column dtype fix. A non-anchor node reaches the row via a hop whose unmatched rows introduce NaN, so the non-empty connected join widens its integer columns to float64. The 0-row path sourced them from the base node frame (int64), so an emptied sum(b.i_col) returned int64 where the non-empty run and master give float64 -- observable through UNION ALL, the same class as the edge-column bug: WHERE a.i_col = 999 RETURN sum(b.i_col) master float64 [] -> int64 [] At 0 rows, cast the integer columns of every node op after the first to float64. The anchor never NaN-widens (stays int); float columns are unchanged. sum/max over an emptied non-anchor node property, and the UNION ALL escape, now match master's dtype. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Wave 37 caught two 0-row schema-fidelity defects in the downstream-node widening added for #31: - Finding 1 (regression): `is_integer_dtype` returns True for nullable `Int64`, so `astype("float64")` collapsed it to numpy float64 -- diverging from both the non-empty run (keeps `Int64`, avg -> `Float64`) and master. Guard the cast to numpy int/uint only, skipping extension dtypes, which hold NA natively and are never widened by the hop. - Finding 2: a downstream node's numpy `bool` column is upcast to `object` by the hop's NaN-introducing left-join in the non-empty path, but the 0-row path left it as raw `bool`. Widen numpy bool -> object to match; nullable `boolean` (extension) stays put via the same guard. Both escape via UNION ALL, both were uncovered by the existing tests (plain int64/float64 only). Add nullable-Int64 and bool/boolean regression tests, mutation-verified against non-empty and master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
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.
Replacement stack for #1714 (rung 2/4), stacked on rung 1.
Scope: connected multi-pattern logical planning, predicate pushdown, and property pruning.
Size: 2 files, +476/-18 (494 changed lines).
Validation: four focused planning tests pass; Ruff and diff gates pass.