fix(gfql): ungrouped aggregates always yield their identity row (#1909) - #1910
Open
lmeyerov wants to merge 5 commits into
Open
fix(gfql): ungrouped aggregates always yield their identity row (#1909)#1910lmeyerov wants to merge 5 commits into
lmeyerov wants to merge 5 commits into
Conversation
openCypher: an aggregate with NO grouping keys always yields exactly one row. "Filter, then count what survived" must return a zero, never an empty frame -- callers doing df['c'][0] were getting an IndexError, or reading 0 rows as "no data" instead of "the count is zero". Fix 1 (silent-wrong, both engines) -- the multi-stage MATCH -> WITH -> RETURN lowering never attached an `empty_result_row`, so any later stage that emptied the stream dropped the identity row: post-WITH WHERE, HAVING-style filter on an aggregate, mid-pipeline LIMIT 0 / SKIP past the end. Wrong for collect (-> []), count(DISTINCT), multi-aggregate RETURNs, post-aggregate expressions (WITH count(*) AS c RETURN c + 1 -> 1) and edge-rooted matches alike. The single-stage MATCH path and the UNWIND row-only path already did this; the discipline is now shared by all three. Fix 2 (same family) -- the row-only path bailed out of synthesizing the identity row entirely whenever the final stage carried SKIP or LIMIT, so `UNWIND [] AS x RETURN count(*) AS c LIMIT 1` returned 0 rows. The rule is synthesize-then-page: LIMIT 0 -> 0 rows, LIMIT n>=1 -> the row, SKIP 0 -> the row, SKIP >=1 -> 0 rows. The MATCH-rooted single-stage path had the mirror bug (LIMIT 0 / SKIP 1 still emitted the identity row) and is fixed the same way. sum() over an empty stream is now 0 rather than null on every path, which retires the round-002 BUG-4 strict xfail. Fix 3 (diagnostics) -- an aggregate introduced by ORDER BY, and unsupported openCypher aggregates (stDev / stDevP / percentileCont / percentileDisc), used to surface as pandas GFQLTypeError [invalid-node-reference] at EXECUTION time and polars NotImplementedError: two different exception types, the wrong code, and the wrong phase for the same rejection. Both are now a compile-time GFQLValidationError [unsupported-cypher-query] with identical text on both engines. ORDER BY that merely NAMES a projected aggregate (`ORDER BY count(*)` or `ORDER BY age + count(*)` next to `count(*) AS cnt`) still resolves to the projected output and is unaffected. Not changed on purpose: sum()/avg() over booleans keeps its numeric coercion. openCypher/Neo4j reject it, but `sum(x > k)` is a core dataframe idiom and the answer is coherent -- pinned as a deliberate, documented extension rather than "conformed" away. The identity-row machinery lives in a new cypher/aggregate_identity.py rather than growing lowering.py by ~200 lines; the surface-guard baseline is bumped 9772 -> 9808 for the residual +36 lines (the compile-time rejections and the shared paging helper, which belong next to the lowering they guard). Residual, pinned strict-xfail: when a WHERE follows the ungrouped aggregate, whether the identity row survives depends on the real aggregate value, which the compiler cannot see; the synthesis declines rather than guess. Three polars NIE families (whole-entity grouping, aggregate over a missing property, ORDER BY after collect-of-collect) are pinned strict-xfail too -- all three are the same missing native polars cypher expression engine, not a local fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
CI measured 83.61%; the audit fails a target file absent from the baseline. Floor recorded at the measured value per convention; the uncovered branches (decline paths + pandas replay fallback) get direct tests next, same as the flatten.py precedent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
The #1909 identity-row module was at 83.61% line coverage (102/122 statements; missing 53, 55-56, 69, 73, 76, 87, 91, 101, 103, 105, 108-111, 128-129, 132, 163, 169) -- every uncovered line was a decline or defensive arm, i.e. exactly the behavior that decides whether a query returns {c: 0} or an empty frame. Adds direct structural tests (precedent: test_flatten_pure_carry_optional.py): seed admits/declines (count_table fast path, key_prefixes, grouped or non-constant keys, malformed aggregation tuples), pass-through projection admits/declines (rename, extend, expression, unknown column), replayed-row scalar normalization including the raising-.item() arm, replay declines (non-allowlisted step, raising pipeline, emptied row), temp stripping, and the top-level suffix arms (ORDER BY/DISTINCT transparent, SKIP/LIMIT paging, non-integer paging value, non-call step, post-aggregate WHERE decline, replay fallback for a post-aggregate expression). Coverage of graphistry/compute/gfql/cypher/aggregate_identity.py: 83.61% -> 100.00% (0 statements missing); the new file reaches 100% on its own, so the floor does not depend on the polars-marked tests in the end-to-end file. Floor in coverage_baselines/ci-pandas-py3.12.json raised 83.61 -> 100.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
Cascading base update after #1907 took its base. One conflicted file. - bin/ci_cypher_surface_guard_baseline.json: took the base's 9733, then --write-baseline. Merged lowering.py is 9769 lines, below this branch's own previous cap of 9808; the ratchet is tightened, not loosened. Everything else merged clean. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. test_aggregate_identity_branches + test_aggregate_identity_row_semantics + test_known_cross_engine_divergences + test_polars_lane_completeness + test_aggregate_type_contract = 508 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
lmeyerov
added a commit
that referenced
this pull request
Aug 15, 2026
Cascading base update after #1910 took its base. Resolutions: - graphistry/compute/gfql/identifiers.py: the diff3 merge base for the hunk is EMPTY and the two sides declare unrelated constants -- this branch's ROW_EDGE_IDENTITY_BASE / EDGE_INDEX_BASE (#1911 safe edge-identity column) and the base's HIDDEN_ALIAS_COLUMN_PREFIX (hidden alias columns). Kept BOTH; both have live importers (row/pipeline.py, polars/row_pipeline.py, gfql_unified.py for this branch's; dataframe/join.py, cypher/reentry/naming.py for the base's), so picking a side would break imports. - bin/ci_cypher_surface_guard_baseline.json: took the base's 9769, then --write-baseline. Merged lowering.py is 9852 lines, below this branch's own previous cap of 9891. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. test_alias_scoping_semantics + tests/compute/gfql/cypher + test_aggregate_identity_row_semantics = 3792 passed, 7 failed -- all 7 [cudf], pre-existing in this GPU-less environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
…guard #1910 went CONFLICTING when #1907's audit landed, so GitHub could build no merge ref and created no workflow runs at all. One code conflict, in lowering.py: this branch DELETES `_row_only_empty_aggregate_row` outright (verified — the branch has neither a definition nor a caller anywhere), while #1907 only stripped the `(#1899)` citation from that function's docstring. Took the deletion; #1907's edit applies to code this PR removes. `lowering_py_max_lines` set to the MEASURED merged count, 9782. The merge also brought in the comment-density guard, which this branch had never run, and it failed on this branch's own debt: - lowering.py comment-block 50 vs cap 49 — the extra was this PR's 2-line block over `_CYPHER_UNSUPPORTED_AGGREGATES`, and it was pure duplication: the name says what it holds, and `test_unsupported_aggregate_functions_are_validation_errors_on_both_engines` already pins the cross-engine same-error contract over all four names. Deleted. - issue-rationale: `aggregate_identity.py` 4 vs cap 0, lowering.py 11 vs cap 10. These docstrings state real contracts, so only the ISSUE CITATIONS are removed (#1899/#1909/#1273/#1711) — the contract prose stays and the test names carry the issues. No cap raised anywhere; the guards now report 17 and 2 files BELOW baseline. ruff clean, all three guards exit 0. Suites: 1655 passed with 7 `[cudf]` failures, all in the recorded pre-existing baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
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.
Stacked on #1907. Round-007's one silent-wrong family, plus two bugs the probe had not seen.
Fix 1 — multi-stage identity row.
compile_cypher_query'swith_stagesbranch only setempty_result_rowfor the shortest-path/OPTIONAL case, so any later stage that emptied the stream (post-WITH WHERE, HAVING,LIMIT 0,SKIP 99) dropped the row openCypher guarantees. New sharedaggregate_identity.pyscans compiled row steps backwards for the last ungrouped aggregate producer, seeds identities (count/sum→0, collect→[], else null), then applies the compiled suffix — symbolically for projections/ORDER BY/DISTINCT/SKIP/LIMIT, via a one-row pandas replay for post-aggregate expressions. It declines when the suffix holds a filter/unwind/join, because survival then depends on the real aggregate value.Fix 2 — paging. The row-only path bailed whenever the final stage had SKIP/LIMIT. Now the identity row is synthesized and paged:
LIMIT 0→0 rows,LIMIT n≥1→the row,SKIP 0→the row,SKIP ≥1→0 rows. Probing found the MATCH-rooted equivalents were not actually correct either (the issue assumed they were):... RETURN count(*) LIMIT 0andSKIP 1both still emitted the row — fixed. Also foundsumidentity was null on the single-stage path; now 0 everywhere.Fix 3 — diagnostics, at compile time on both engines. Unsupported aggregates (
stDev,percentileCont, …) rejected in the expression-constraint walker; aggregate-in-ORDER-BY rejected via a dedicated check that runs after projected-output rewriting, soORDER BY count(*)alongsidecount(*) AS cntstill resolves (an over-rejection this caught in a first attempt). Both now raiseGFQLValidationError [unsupported-cypher-query]with byte-identical messages across engines, replacing execution-timeinvalid-node-reference/NotImplementedError.BUG-4 retired:
test_sum_over_empty_match_is_zerowas a strict xfail from round-002; thesum→0identity makes it pass, marker removed with openCypher 9 §3.2 quoted in the docstring. That is the only existing-test change.Kept deliberately:
sum()/avg()over booleans stays permissive (useful dataframe-native idiom, coherent answer) — now pinned as a documented extension rather than left ambient.96 new pins (91 passed / 5 strict-xfail). polars NIE families pinned rather than served: all three reduce to one root cause (no native cypher expression engine in the polars row pipeline), each paired with a pandas test asserting the hand oracle so the pins flip loudly when polars lands. Residual pinned: post-aggregate
WHEREon the aggregate value is undecidable at compile time.Gates: full-tree failure set byte-identical, TCK 4144P/0F (companion branch pushed), typecheck 329 files, ruff + hygiene guards clean, surface baseline 9772→9808 (≈200 lines were extracted to a new module specifically to keep lowering.py from absorbing them). Compile-time A/B: hot lanes unchanged; only post-aggregate-expression queries pay the one-row replay (compile-time, cached). No two-hop/self-loop perf code touched.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi