Commit 674443e
perf(gfql/polars): make the chain combine proportional to the traversal result (#1785)
* perf(gfql/polars): make the chain combine proportional to the traversal result
Two graph-sized terms sat inside a combine whose answer is a handful of rows.
1. `_combine_edges` ran the prev/next endpoint gates for EVERY step, including the
node steps whose edge frame is `g._edges.clear()` -- zero rows. The eager combine
skipped those; the collect-once (Track B) rewrite lazified the step frames and
`.lazy()` erases the height, so the skip silently went dead. The cost lands on the
side that is NOT empty: for the first step the gate's key side is the whole node
table, and polars builds the hash table on that side before discovering the probe
side has no rows. Isolated in raw polars: 6.99 ms for one such join at N=2M vs
0.22 ms against a one-row key side -- and a chain pays one per node step.
`_LazyShim.step` now records the row count while the frame is still eager and an
empty step is dropped from the id union. It can contribute no ids, so the result is
unchanged by construction. The skip keys on KNOWN-empty only: a frame that arrives
already lazy reports no height and is planned normally.
2. The output node rows were materialized in TWO passes over the node table -- one for
the ids the steps kept, one more for the surviving edges' endpoints the first pass
missed -- then concatenated. The output node set is the UNION of those two id sides,
so `_materialize_node_rows` unions the ids first and reads the node table ONCE. The
row-level `unique(subset=[node])` is preserved verbatim: those rows go on to feed
`how="left"` alias joins where a node table carrying the same id twice multiplies
rows. The id sides themselves are NOT deduplicated -- they are semi key sides.
Note for the record: the recorded description of this residual ("UNIQUE over a UNION of
the per-step FULL frames") named the wrong operator. The union inputs are 1-row step
frames; the `unique` costs 0.008 ms. It was the union BRANCHES -- empty-probe joins
against graph-sized build sides -- exactly as the earlier semi-dedup finding.
MEASURED, one dimension at a time, synthetic LDBC-IS5-shaped graphs (one-row answer,
polars-engine resident indexes, local CPU, min of 9):
vary NODES at E=2M 250k -> 4M: 10.17 -> 45.48 ms before
7.60 -> 17.17 ms after (2.65x at 4M)
node-count slope 3.7x flatter (+35.3 ms -> +9.6 ms over 16x N)
vary EDGES at N=1M 500k -> 8M: 13.47 -> 26.02 ms before
7.08 -> 17.57 ms after
edge slope essentially unchanged, as expected for a node-side fix; constant ~6 ms lower
same 2-3x holds for n-e-n, n-e-n-e-n and undirected shapes
PARITY: 280 shape x graph combinations (clean / self-loop / dangling / duplicate keys /
multi-edge / back-edge / isolated) and 400 duplicate-node-id combinations x 5 repeats
compared as FULL frames including row order -- 0 diffs vs the pre-change tree, and
stable across repeated runs. `graphistry/tests/compute/` failure set is byte-identical
before and after (119 pre-existing failures, 0 new, +119 passed = the new file).
TESTS pin the boundary, not a wall clock. Mutation-checked; each row is a deliberate
reintroduction of a bug, run against the new file:
drop the empty-step skip -> 2 fail (both structural tests)
restore the two-pass materialization -> 1 fail (node table read 2x)
drop the row-level node dedup -> 18 fail (parity + dedup + order)
drop the endpoint fold -> 2 fail
drop the node order restore -> 34 fail
skip steps whose height is UNKNOWN -> 1 fail
NOT caught, and reported as such: dropping the edge order restore, and dropping
`maintain_order` on the node dedup. Neither is observable -- the edge semi-join returns
EORD order naturally at 5k/60k/300k across 5 shapes, and the node semi-join feeding the
dedup is unordered either way. Both are pre-existing defensive code, untouched here.
Also honest about coverage: the endpoint fold could not be shown load-bearing
end-to-end (6300 generated chain executions found no graph/shape where the step ids miss
an edge endpoint), so it is tested at the helper, where the case can be constructed --
an end-to-end test of it would have passed with the endpoint side removed entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VARwnAqmvczhz5EmP7TYyZ
* review(#1785): pin the edge sort under streaming; scope the survivor claim; kill the twin guard
Review skill found three items worth fixing; all three were claim-scope or
follow-up-hazard rather than defects in the shipped behaviour.
1. `sort(EORD)` is NOT dead code — the probe that called it unpinned was engine
blind. It only exercised the in-memory collect, where the frame comes back
EORD-ordered even with the sort deleted. Under `GFQL_POLARS_CPU_STREAMING=1`
it does not, and a trailing rows(limit=)/skip would then slice the wrong rows.
The existing 60k order test is now parametrized over the collect engine.
Mutation-checked: deleting the sort fails [streaming] and PASSES [in-memory],
which is exactly why the original probe saw nothing.
2. The `maintain_order=True` docstring claimed the surviving duplicate is decided
"the same way it was before", citing a 400-combo A/B. That A/B ran on the
default engine only; under streaming the survivor genuinely differs. Scoped the
claim to the in-memory collect and said plainly that the survivor is a stable
property there, not a guaranteed one.
3. `_apply_node_names` still carried the ORIGINAL spelling of the bug this branch
exists to fix: `is_lazy(df) or df.height > 0`, where the function is always
called with lazified steps, so `is_lazy` short-circuits True and the height test
is unreachable. Restated against `edges_empty`, which survives lazification.
Unlike the edges combine this guard is SEMANTIC, not a cost guard — an empty
next edge step must not empty `named` through the gate below it. I could not
make the dead version observable, so this is hardening, not a bug fix; but
leaving a known-dead cardinality guard beside a freshly-revived one is precisely
how the original defect recurred.
lint + mypy clean; 1159 polars chain/combine/semi-dedup tests pass.
* review(#1785): type the narrow-combine helpers instead of leaving them inferred
The helpers this PR added or reshaped were untyped, which violates the repo's
engine-agnostic typing convention. Annotate them with the aliases rather than
bare Any: `_LazyShim.__init__` / `.step`, `_known_empty`, `_combine_node_ids`,
`_materialize_node_rows`, plus class-level slot annotations on `_LazyShim`
(bare annotations only — a class-level value would collide with __slots__).
The slots are `Optional[pl.LazyFrame]`, NOT the `PolarsFrame` union: every
construction site calls `.lazy()` first, which is the point of the shim, and
`pl.concat`'s TypeVar rejects a `DataFrame | LazyFrame` argument outright. The
union spelling type-checks as 7 errors; the LazyFrame spelling as none.
`_combine_node_ids` takes the `_LazyShim`, not a `Plottable` — the call site
passes `g_lz`, a duck-typed stand-in that is not a Plottable subclass.
Two consequences worth naming:
* `_known_empty` needs an explicit cast for `.height`. `is_lazy` is a plain
bool predicate, so mypy cannot narrow its else-branch. Widening it to a
`TypeIs` would narrow this for every caller in the engine, but that is a
dtypes.py-wide change and not this PR's.
* `collect_all`'s results are rebound to new names. `final_nodes` is
statically a LazyFrame down that block and `collect_all` returns eager
frames, so reusing the name is a type error — and silencing it would cost
exactly the lazy/eager distinction the shim exists to preserve.
Sibling helpers in the same file (`_semi`, `_align_seed_dtype`, ...) are also
untyped but pre-existing, and are left alone.
Verified in-container on dgx-spark, differentially: mypy reports 213 errors
with 1 in chain.py both before and after this commit — the same pre-existing
`maybe_index_hop` arg-type — so this adds none. (The container's pandas-stubs
drift from the CI lockfile makes the absolute count meaningless; only the
differential is evidence.) The gfql suite with `--gpus all` is byte-identical
to the pre-commit tree: 5570 passed, 25 skipped, 15 xfailed, 0 failed on both.
ruff clean under the repo config.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>1 parent 8483598 commit 674443e
3 files changed
Lines changed: 521 additions & 24 deletions
File tree
- graphistry
- compute/gfql/lazy/engine/polars
- tests/compute/gfql
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| 15 | + | |
15 | 16 | | |
16 | 17 | | |
17 | 18 | | |
| |||
0 commit comments