Skip to content

Commit 8d8b9d9

Browse files
committed
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.
1 parent ebfa720 commit 8d8b9d9

2 files changed

Lines changed: 31 additions & 6 deletions

File tree

graphistry/compute/gfql/lazy/engine/polars/chain.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,11 @@ def _materialize_node_rows(all_nodes, step_ids, endpoint_ids_frame, node_col):
358358
Row ORDER out of here is arbitrary — a polars semi-join does not preserve left-frame order —
359359
and the caller restores input-frame order with an explicit sort. ``maintain_order`` is kept
360360
verbatim from the pre-refactor call so that WHICH duplicate row survives is decided the same
361-
way it was before (A/B over 400 duplicate-id combos: identical full frames)."""
361+
way it was before: A/B over 400 duplicate-id combos gives identical full frames **under the
362+
default in-memory collect**. Scoped deliberately — under streaming collect the survivor DOES
363+
differ from the pre-refactor call (measured), so it is not a guaranteed property of this
364+
helper, only a stable one on the default engine. Anything needing a specific survivor must
365+
order explicitly rather than rely on this."""
362366
import polars as pl
363367
ids = pl.concat([step_ids, endpoint_ids_frame], how="vertical_relaxed")
364368
return all_nodes.join(ids, on=node_col, how="semi").unique(
@@ -399,7 +403,16 @@ def _apply_node_names(out, g, steps, auto_hop_col: str = _AUTO_NODE_HOP):
399403
on=node_col, how="semi")
400404
if idx + 1 < len(step_list):
401405
next_op, next_step = step_list[idx + 1]
402-
if isinstance(next_op, ASTEdge) and next_step._edges is not None and (is_lazy(next_step._edges) or next_step._edges.height > 0):
406+
# Cardinality guard, restated against a fact that SURVIVES lazification. The old
407+
# spelling was `is_lazy(df) or df.height > 0`, and `_apply_node_names` is always
408+
# called with lazified steps — so `is_lazy` short-circuited True and the height
409+
# test was unreachable. That is the identical silent death this commit fixes one
410+
# function above; leaving a second copy of it here is how the bug recurs.
411+
# Unlike the edges combine this one is SEMANTIC, not a cost guard: an empty next
412+
# edge step must not empty `named` via the gate below.
413+
next_edges_empty = getattr(next_step, "edges_empty", None)
414+
if (isinstance(next_op, ASTEdge) and next_step._edges is not None
415+
and next_edges_empty is not True):
403416
e = next_step._edges
404417
if next_op.direction == "forward":
405418
part = e.select(pl.col(src).alias(node_col))

graphistry/tests/compute/gfql/test_engine_polars_narrow_combine.py

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,11 +337,19 @@ def test_duplicate_node_ids_still_collapse_to_one_row():
337337
assert graph_sig(g_pd.chain(chain, engine="pandas")) == graph_sig(out)
338338

339339

340-
def test_output_row_order_survives_a_frame_big_enough_to_parallelize():
340+
@pytest.mark.parametrize("streaming", [False, True], ids=["in-memory", "streaming"])
341+
def test_output_row_order_survives_a_frame_big_enough_to_parallelize(streaming):
341342
"""Order at fixture scale proves little: polars' joins happen to come back in left order on a
342343
handful of rows and only reorder once the hash join actually runs in parallel. Use a frame
343344
large enough to reorder, shuffled so input order is not id order, and pin that the combine's
344-
explicit sorts put both output frames back into input-frame order."""
345+
explicit sorts put both output frames back into input-frame order.
346+
347+
Parametrized over the collect engine because the IN-MEMORY engine hides the edge sort: with
348+
`final_edges.sort(EORD)` deleted, in-memory still returns EORD-ordered rows at every size
349+
probed, so an in-memory-only test would call that sort dead code. Under STREAMING it does
350+
not, and a trailing rows(limit=)/skip would then slice the wrong rows. Both engines here,
351+
so neither sort can be removed on the strength of the other's silence."""
352+
from graphistry.compute.gfql.lazy import set_cpu_streaming
345353
size = 60_000
346354
rng = list(range(size))
347355
order = rng[1::2] + rng[0::2][::-1] # deterministic shuffle
@@ -350,8 +358,12 @@ def test_output_row_order_survives_a_frame_big_enough_to_parallelize():
350358
"d": [(k * 13 + 1) % size for k in order],
351359
"type": ["K" if k % 2 else "L" for k in order]})
352360
_, g_pl = _pair(nodes, edges)
353-
out = g_pl.chain([n({"grp": 1}, name="m"), e_forward({"type": "K"}, name="r"), n(name="p")],
354-
engine="polars")
361+
set_cpu_streaming(streaming)
362+
try:
363+
out = g_pl.chain([n({"grp": 1}, name="m"), e_forward({"type": "K"}, name="r"), n(name="p")],
364+
engine="polars")
365+
finally:
366+
set_cpu_streaming(None)
355367
assert out._nodes.height > 1000 and out._edges.height > 1000, "fixture stopped being big"
356368

357369
node_rank = {k: i for i, k in enumerate(nodes["key"].tolist())}

0 commit comments

Comments
 (0)