Skip to content

fix(gfql): treat binding seed ids as identities - #2001

Merged
lmeyerov merged 4 commits into
masterfrom
fix/gfql-1996-binding-seed-identity
Aug 28, 2026
Merged

lmeyerov merged 4 commits into
masterfrom
fix/gfql-1996-binding-seed-identity

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • treat node ids, not duplicate physical node-table rows, as identities when generic binding traversal starts
  • preserve first-encounter order in pandas/cuDF and Polars seed state
  • keep relationship rows as the source of MATCH bag multiplicity, including genuine parallel edges

Correctness oracle

The fixture duplicates storage rows for node ids 1 and 2 but adds no graph identities or relationships. Its hand-derived grouped counts are LA=6, NY=2, SF=1; adding one parallel 1 -> 5 relationship changes only LA to 7. Before this fix the generic route returned 10/3/1 and 12/3/1 by multiplying paths from duplicate seed rows.

Semantics boundary

GQL-conformant Cypher preserves binding-row duplicates for plain RETURN / RETURN ALL; only DISTINCT deduplicates (Neo4j Cypher RETURN). This change follows that boundary:

  • duplicate physical node-table rows with the same node id collapse only at the initial identity wavefront
  • relationship match rows are never collapsed by this change
  • a genuine parallel edge therefore remains an additional match row and changes LA from 6 to 7

Validation

  • exact reviewed head: a888c33f2d42f626bb0754fb1186b2d1c1bf6561
  • topology: four single-parent commits directly on master@d509e428b0b63a7dd6d92ee5271e75afd15ca2bf; zero merge commits
  • incremental diff: four intended files, +94/-6; whitespace clean
  • prohibited added-line audit: zero Any, object, cast, getattr, setattr, hasattr, or unqualified List/Dict; finite engine values use Literal
  • focused safe CPU regression: 4 passed / 4 GPU cells deliberately deselected
  • ./bin/lint.sh: syntax, Ruff, type-hygiene, comment-density, and relative-import guards clean
  • ./bin/typecheck.sh: mypy 2.3.1, 336 source files, zero issues
  • no Docker, local GPU, or broad local suite; the current GitHub Checks panel is authoritative for the full matrix

Review resolutions

  • Stacking: the stale predecessor merges were removed. The branch is now four linear commits directly on landed fix(gfql): preserve bag multiplicity for whole-entity projections #2000 master.
  • Positive/negative boundary: duplicate physical seed rows are the positive case that must collapse; a real parallel relationship row is the negative control that must remain multiplicative. Both cases run across the engine matrix.
  • Anti-vacuity without dynamic patching: the regression uses semantically neutral SKIP 0 to select the generic route and assert_fast_path(..., served=False) to prove the grouped fast path was consulted and declined.

Closes #1996

Reviewer handoff

Please review the incremental four-file diff for:

  1. seed-state deduplication is by node id and preserves first encounter order
  2. relationship rows, including parallel edges, still determine MATCH bag multiplicity
  3. the regression proves the generic route without monkeypatch.setattr or another dynamic-attribute escape

Stack / landing

@lmeyerov

Copy link
Copy Markdown
Contributor Author

Is incorrectly stacked?

@lmeyerov
lmeyerov changed the base branch from master to fix/gfql-1994-whole-entity-bag August 21, 2026 20:11
return None


def _run_generic(engine: _GFQLEngine, *, parallel_edge: bool) -> typing.Mapping[str, int]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sufficient, positive and negative testing on either side of the boundary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes—the regression now pins both sides of the boundary. Duplicate physical node-table rows for ids 1/2 are the positive case: the initial traversal wavefront must collapse them as the same graph identities (LA=6, NY=2, SF=1). A genuine parallel 1 -> 5 relationship is the negative control: relationship match rows must not be deduplicated, so only LA increases to 7. Both cases run across the engine matrix. I also removed monkeypatch.setattr: neutral SKIP 0 selects the generic route, and assert_fast_path(..., served=False) proves the grouped fast path was consulted and declined, so the value assertion cannot pass vacuously.

@lmeyerov
lmeyerov force-pushed the fix/gfql-1994-whole-entity-bag branch from 9de0b68 to 6963275 Compare August 27, 2026 08:54
@lmeyerov
lmeyerov force-pushed the fix/gfql-1996-binding-seed-identity branch from 01b0c66 to a888c33 Compare August 27, 2026 09:52
@lmeyerov
lmeyerov changed the base branch from fix/gfql-1994-whole-entity-bag to master August 27, 2026 09:53
@lmeyerov

Copy link
Copy Markdown
Contributor Author

Resolved. The stack was stale after #2000 was rewritten and landed. #2001 is now rebased directly onto master@d509e428b0b63a7dd6d92ee5271e75afd15ca2bf at exact head a888c33f2d42f626bb0754fb1186b2d1c1bf6561: four single-parent commits, zero merges, and only the four #2001 files in the incremental diff. Successor #2002 will be rebased only after this exact head lands.

@lmeyerov
lmeyerov merged commit d627fac into master Aug 28, 2026
76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GFQL: polars-gpu answers a different count() than every other engine on duplicate node-id rows (fast-path vs generic-route divergence)

1 participant