Repository navigation
perf(gfql): pay #2039's column restore only when an alias can shadow a column - #2104
Merged
Merged
Conversation
…a column #2039 fixed a real bug: an edge alias sharing its name with a column its own step filters on was stamped as a boolean marker, and the backward pass then re-filtered that marker as the column and raised. The repair re-joins the graph's WHOLE edge frame against each step's edge ids to recover the original columns. That restore is unconditional, and it is expensive: on LiveJournal it semi-joins 34,681,189 edge rows, twice per hop. Bisected on dgx against the ladder's bulk hop, 3 reps per tree, identical result sizes, load-gated, one job at a time: 6417cdd (parent) hop1 351.1, 328.0, 348.9 CLEAN a599909 (#2039) hop1 421.8, 438.9, 468.3 REGRESSED <- disjoint The shadowing needs an ASTEdge alias NAMED like an existing edge column. When no alias can collide, the step's own edges already carry the graph's columns and the restore is pure cost. _edge_alias_can_shadow_column decides that once per chain; unprovable inputs (no edge frame, unreadable columns) answer True and keep the restore, so the guard fails toward correctness. Measured with the guard, same lane: hop1 403.9, 423.2, 426.1 -> 332.8, 269.1, 280.9 (-33%, back inside the pre-#2039 band 259.5-351.8, disjoint from the regressed band) hop2 7347.5, 7366.4 -> 6888.5, 6742.5, 6902.5 Not a revert: #2039's collision pins, the alias-scoping suite and #2051's dedup pins all pass, with a failure set identical to the unmodified tree (34, every one a pre-existing cuDF env artifact). The guard is verified to ENGAGE and to still fire on real collisions rather than being green over dead code. q1-q9 at 100k are unchanged (they never regressed): q1 22.92->23.02, q8 12.90->13.28, q3 8.96->8.05, all within run-to-run spread with identical rows. Found only because the GraphFrames ladder was being re-measured; no board in CI exercises a bulk hop over tens of millions of edges, so this sat undetected through the 0.60 release. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_012Me1E7ZdDuGqJGu3mMEzhp
The comment-encoding guard requires a contract stated by NAME, with the issue carried by the pin's test name. Drops two issue refs from the source; the behaviour pins in test_chain_alias_shadow_restore_guard.py carry the provenance. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_012Me1E7ZdDuGqJGu3mMEzhp
Two CI guards caught the same omission: the module calls engine='polars' but ran in
test-core-python and test-gfql-core, where polars is not installed, and
test_polars_lane_completeness flagged that it belonged to no polars lane at all.
Adds the pytest.importorskip('polars') the sibling polars modules use, and registers the file
in POLARS_TEST_FILES so the pins actually execute somewhere rather than silently skipping.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012Me1E7ZdDuGqJGu3mMEzhp
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 #2102.
The regression
#2039 fixed a real bug — an edge alias sharing its name with a column its own step filters on was stamped as a boolean marker, and the backward pass then re-filtered that marker as the column and raised. The repair re-joins the graph's whole edge frame against each step's edge ids to recover the original columns:
It runs unconditionally, in two places (backward pass and pruned re-execution). On LiveJournal that is a semi-join over 34,681,189 edge rows, twice per hop.
Bisected on dgx against the ladder's bulk hop — 3 reps per tree, identical result sizes, load-gated, one job at a time:
6417cdd30(parent)a59990981(#2039)Max clean 351.1 < min regressed 421.8 — disjoint.
The fix is a guard, not a revert
The shadowing needs an
ASTEdgealias named like an existing edge column. When no alias can collide, the step's own edges already carry the graph's columns and the restore is pure cost._edge_alias_can_shadow_column(ops, g)decides once per chain.It fails toward correctness: unprovable inputs (no edge frame, unreadable columns) answer
Trueand keep the restore.Measured, same lane
−33% on hop1, back inside the pre-#2039 clean band (259.5–351.8) and disjoint from the regressed band. hop2 recovers to at/below its clean band.
Correctness
rel,src,dstkeep it. New pins cover both sides of that boundary, plus the no-edge-frame fail-safe and that the colliding chain still answers.Why it went unnoticed
No board in CI or the release suite exercises a bulk hop over tens of millions of edges. It surfaced only because the GraphFrames ladder — the one board that does — was being re-measured after being waived as stale. The waiver was hiding a live regression.
🤖 Generated with Claude Code
https://claude.ai/code/session_012Me1E7ZdDuGqJGu3mMEzhp