perf(gfql): identity-stable _pl_nan_to_null + clean-cache (kills O(E) per-hop NaN re-scan on polars)#1735
Merged
Merged
Conversation
… per-hop NaN re-scan on polars) Every hop() runs _coerce_input_formats -> _pl_nan_to_null, which scanned is_nan().any() over every float column of the (unchanged) resident edge frame ON EVERY CALL. So repeated seeded hops on a resident graph (seeded-Search / native-hop) grew O(E) with edge count on polars/polars-gpu while pandas stayed flat. _pl_nan_to_null now probes an eager polars frame for real NaN once, returns a clean frame UNCHANGED (restores the #1726 identity guard reverted by #1731 -> frame-identity caches like the #1658 index keep engaging), rewrites only columns that genuinely carry NaN (values identical to the old unconditional fill_nan), and caches the id of clean frames so later calls skip the probe. Recycle-safe via weakref.finalize (a rebound/new frame re-probes -> NaN->null semantics unchanged). Measured (dgx): indexed polars g.hop on 8M edges + one float column: 33.8ms (growing O(E)) -> 0.20ms FLAT (~140x, O(degree)), matching pandas. This is the per-call cost that made polars/polars-gpu seeded LDBC Search grow with scale and lose to Kuzu. Regression: 184 passed (full 4-engine test_index.py parity + test_engine_coercion) — the one failure (test_chain_dask_edges) is pre-existing on clean master (RAPIDS-image dask issue). New TestPlNanCleanCache: NaN-present cleaned, clean-frame cached same-object, distinct frames independent. Subsumes #1733 (identity-stable reland) and adds the clean-cache. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
This was referenced Jul 19, 2026
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 20:32
Contributor
Author
|
STACK SEPARATION (2026-07-19, for manual review): READY FOR REVIEW (stable, low-churn, land bottom-up):
EXPERIMENTAL (high-churn, do NOT land yet — iterating with the benchmark campaign): pyg-bench #90 (runner/config migration) is independent and review-ready. Whole-stack validation on dgx at the #1738 tip after today's lint fixes + cascade rebase: 362 passed, 1 pre-existing failure ( |
lmeyerov
commented
Jul 19, 2026
| _PL_NAN_CLEAN_IDS: Set[int] = set() | ||
|
|
||
|
|
||
| def _mark_pl_nan_clean(df) -> None: |
Engine.py sits outside graphistry/compute (the polars lane's --cov scope) and the core coverage lane has no polars installed, so _pl_nan_to_null's polars branches could execute in no lane that also records them — the changed-line gate saw 16.67%. Add a lane phase running test_engine_coercion.py with --cov=graphistry.Engine --cov-append. Local repro at PR head: 18/18 changed executable lines covered (100%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
…olars (review) Review feedback on #1735: the NaN-clean helpers are polars-engine logic, noisy in Engine.py, and were invisible to every coverage lane that could execute them. Move them to graphistry/compute/gfql/lazy/engine/polars/nan_clean.py with precise static types (PolarsT eager-in/eager-out, pl.DataFrame for the cache marker), local-import from Engine.df_to_engine/ComputeMixin (no cycle: lazy/ never imports Engine at load). Tests move to gfql/test_polars_nan_clean.py (+ gc-eviction + LazyFrame cases), join the polars lane list; the interim Engine-scoped coverage phase in test-polars.sh is no longer needed. Audit baseline gains the new file (floor 90). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov
added a commit
that referenced
this pull request
Jul 19, 2026
…a (deferred #1735 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov
added a commit
that referenced
this pull request
Jul 20, 2026
…iguation + identity-key + key_prefixes (LDBC IC4 native) (#1737) * feat(gfql): polars whole-entity aggregation Cypher (HAS_label disambiguation + identity-key + key_prefixes) Three pandas-parity ports unlock the LDBC IC4 shape natively on polars: binding_rows_polars applies the HAS_<Label> candidate-domain narrowing (only under duplicated candidate ids, mirroring pandas exactly) instead of a blanket decline; alias.__gfql_node_id__ resolves to the bare alias id column; and group_by expands key_prefixes like pandas. Official IC4 runs parity-exact (harness ok @127.8ms SF0.1, was NIE). 184 suite tests + 2 new pass on dgx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * fix(gfql): match pandas disambiguation call-site exactly (reached-domain dup probe, forward single-hop gate) Self-review of the initial port found two parity divergences vs the pandas call site (row/pipeline.py:3670): pandas probes id duplicates on the REACHED candidate domain (candidate_source ∩ wavefront), not the full node table, and applies the narrowing only for forward single-hop edges. The polars twin now runs after the hop's state semi-join with the same gates; two new engine-parametrized tests pin both branches (global-dup-but-unique-reached must NOT narrow; reached-collision must narrow). 186+2 tests pass on dgx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * refactor(gfql): TypedDict-narrow group_by params + update stale whole-row groupby test (review) Review feedback on #1737: - op.params is wire-format Dict[str, Any] keyed by the runtime op.function, so precise typing = per-function TypedDicts narrowed at dispatch; adds _GroupByParams for the group_by site this PR touches (full tagged-union ASTCall params is a larger AST refactor, out of scope here). - test_try_native_row_op_declines_whole_row_group_prefixes asserted the pre-#1737 decline behavior; whole-row (prefixed-key) grouping now lowers natively, so the test asserts the native result against the hand-derived oracle instead (was the test-polars CI failure). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * ci(polars): run entity-groupby tests in the polars lane (changed-line gate) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * refactor(gfql): precise wire-format types for polars row-pipeline sigs (review) Replace Sequence[Any] with SelectItem/OrderKey/AggSpec aliases across select/order_by/group_by lowering; group_by_polars now takes Sequence[str] keys + Sequence[AggSpec] + Optional[Sequence[str]] key_prefixes (matching the pandas oracle sig); _GroupByParams.aggregations narrowed to List[AggSpec]; _agg_expr schema typed as Mapping[str, pl.DataType]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * docs(gfql): nan-clean cache soundness notes + LazyFrame collect_schema (deferred #1735 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * fix(gfql): HAS_<Label> dup-id graphs decline to honest NIE in binding_rows_polars (review F1) The disambiguation gate probed the chain-combine node table, which is already deduplicated by id — the duplicate probe could never fire, and with the old decline removed, duplicate-id graphs returned silently row-order-dependent first-occurrence rows where pandas narrows to the HAS_<Label> label row (reproduced end-to-end). The label row pandas narrows to may already be gone from the deduped table, so a faithful native answer is impossible here: probe the pre-chain base graph (_gfql_rows_base_graph) instead — unique-id graphs stay native (pandas would not narrow either; IC4 keeps its win), duplicate-id graphs decline to the honest NIE. Also from review: binding-rows-path regression tests that actually reach the gate (dup-id NIE both row orders + unique-id parity), key_prefixes genuine expansion/overlap test, malformed-AggSpec decline test, nan_clean soundness comment corrected (polars does have in-place mutation APIs; state the real no-mutation contract), AggSpec import made runtime (introspectable TypedDict), lower_select_items/lower_order_by_keys return List[pl.Expr]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * refactor(gfql): hoist SelectItem/OrderKey/AggSpec to engine-neutral call/support (review) The wire-format payload types describe ASTCall.params for every engine, not just polars: move them next to their runtime validators in gfql/call/support.py (single source of truth, kept in lockstep) and adopt them in the engine-neutral consumers — pandas RowPipelineMixin.group_by, the ast.group_by builder, and the cypher lowering's aggregation lists (TYPE_CHECKING imports where the call package __init__ would cycle). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL * chore(gfql): bump cypher surface-guard baseline 9244->9249 Pure typing growth in cypher/lowering.py from the engine-neutral SelectItem/OrderKey/AggSpec hoist (TYPE_CHECKING import block + List["AggSpec"] annotations at 3 call sites). No new compiled-surface fields or properties. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 tasks
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
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.
Summary
Every
hop()runs_coerce_input_formats→_pl_nan_to_null, which scannedis_nan().any()over every float column of the (unchanged) resident edge frame on every call. Repeated seeded hops on a resident graph — the seeded-Search / native-hop pattern — therefore grew O(E) with edge count on polars/polars-gpu, while pandas stayed flat. This is the dominant per-call cost that made polars/polars-gpu seeded LDBC Search grow with scale and lose to Kuzu.Change (
graphistry/Engine.py,_pl_nan_to_null)fill_nan).weakref.finalize(a rebound/new frame re-probes → NaN→null semantics unchanged).Measured (dgx-spark)
Indexed polars
g.hopon 8M edges with one float column: 33.8 ms (growing O(E)) → 0.20 ms FLAT (~140×, O(degree)), matching pandas. pandas unchanged (flat). polars no-float hop still grows mildly (0.17→0.91 ms) — a smaller, separate per-call cost, out of scope here.Validation
test_index.pyparity +test_engine_coercion.py. The one failure (test_chain_dask_edges) is pre-existing on clean master (RAPIDS-image dask issue; verified on the unpatched checkout).TestPlNanCleanCache: NaN-present cleaned to null; clean float frame returned same-object + cached; distinct frames don't cross-contaminate (all 4 pass).Relationship to #1733
Subsumes PR #1733 (the #1726 identity-stable reland) — this does the identity-stable behavior and adds the clean-cache. #1733 can be closed in favor of this, or this rebased on it.
Impact
Makes polars/polars-gpu seeded Search O(degree) on float-column (i.e. real) graphs — the contained lever toward a 4-engine LDBC Search win. Broadly speeds all polars Search that re-hops a resident graph.
🤖 Generated with Claude Code