Skip to content

Commit 24ce01f

Browse files
committed
review(#1783): fix a silent no-op on float indexes; drop a dead guard; pin edge order
Review skill found no correctness defect (the adversarial question — is how='left' really the only consumer of _lean_prefilter_right — came back CONFIRMED: one non-test caller, the local is dead on the next line, and every other merge in chain.py builds its right side independently). These are the quality items. 1. `right[0:0]` is LABEL-based slicing on a float index — pandas routes those through slice_indexer — so it returned ONE row and the shrink silently did nothing. Now `right.iloc[0:0]`, positional on every index type. The RESULT was always correct (a 0-row left yields a 0-row how='left' merge either way), which is why this needed its own test: the bug was an invisible perf no-op, not a wrong answer. Mutation-checked — reverting to `[0:0]` fails the new test. 2. Docstring said "Only shrinks when `left` is materially smaller", which the new empty-left branch contradicts. Restated, and now records that the whole helper is pandas-only in practice (`_lean_engine_ok` is checked first), so there is no cross-engine exposure. 3. Removed `if added_edge_index and EID != EORD:` — dead by construction, since the only branch setting added_edge_index also sets EORD = EID. lint clean; 1022 lean-combine + polars-chain tests pass.
1 parent 7ea421a commit 24ce01f

3 files changed

Lines changed: 33 additions & 5 deletions

File tree

graphistry/compute/chain_lean_combine.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,12 @@ def _lean_prefilter_right(left: DataFrameT, right: DataFrameT, key: str, engine:
9191
"""Shrink ``right`` to the keys present in ``left`` before a ``how='left'``
9292
merge. A left merge discards unmatched ``right`` rows anyway, so this is
9393
byte-identical (row order = ``left`` order; matched right rows preserved,
94-
including any fan-out). Only shrinks when ``left`` is materially smaller.
94+
including any fan-out). Shrinks when ``left`` is EMPTY (nothing can match) or
95+
materially smaller than ``right``; otherwise the membership pass costs more
96+
than the join it would save.
97+
98+
pandas-only in practice: ``_lean_engine_ok`` is checked first and returns
99+
``right`` unchanged for cuDF / polars / dask, so nothing here is cross-engine.
95100
"""
96101
if not _lean_combine_enabled() or not _lean_engine_ok(engine):
97102
return right
@@ -111,7 +116,11 @@ def _lean_prefilter_right(left: DataFrameT, right: DataFrameT, key: str, engine:
111116
# correct, and it was the one case the gate below declined: an empty
112117
# intermediate against a graph-sized side (measured: a 0-row left joined
113118
# against 14M edges dominated a 112ms single-node query).
114-
return right[0:0]
119+
# `.iloc` and not `right[0:0]`: bare slicing is LABEL-based on a float index
120+
# (pandas routes those through slice_indexer), so `right[0:0]` returns ONE row
121+
# there and the optimization silently does nothing. `.iloc` is positional on
122+
# every index type.
123+
return right.iloc[0:0]
115124
if left_len * _LEAN_SHRINK_RATIO > right_len:
116125
return right
117126
return right[right[key].isin(left[key])]

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -955,10 +955,10 @@ def _plain_edge(op):
955955
final_nodes = _apply_node_names(final_nodes, g_lz, steps_lz, auto_hop_col=auto_hop_col)
956956

957957
final_nodes = final_nodes.sort(NORD).drop(NORD)
958+
# EORD IS EID whenever we synthesized the id (that is the point of this change), so the
959+
# single drop above removes it. There is no `added_edge_index and EID != EORD` case left
960+
# to handle: the only branch that sets added_edge_index also sets EORD = EID.
958961
final_edges = final_edges.sort(EORD).drop(EORD)
959-
if added_edge_index and EID != EORD:
960-
# When EORD IS EID the drop above already removed it (same column, one drop).
961-
final_edges = final_edges.drop(EID)
962962
final_edges, final_nodes = collect_all([final_edges, final_nodes])
963963
final_edges = _restore_edge_dtypes(final_edges, src, dst, _endpoint_restore)
964964
return self.nodes(final_nodes, node_col).edges(final_edges, src, dst)

graphistry/tests/compute/test_chain_lean_combine.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,3 +292,22 @@ def test_empty_left_shrink_holds_through_the_chain():
292292
on._nodes.sort_values(list(on._nodes.columns)).reset_index(drop=True),
293293
)
294294
assert len(on._nodes) == 0
295+
296+
297+
def test_empty_left_shrink_works_on_a_float_index():
298+
"""`right[0:0]` is LABEL-based on a float index — pandas routes those through
299+
slice_indexer — so the bare-slice spelling returns ONE row there and the shrink
300+
silently does nothing. The result stays correct either way (a 0-row left still yields
301+
a 0-row how='left' merge), which is exactly why this needs its own test: the bug is a
302+
silent perf no-op, invisible to any assertion about the merge result."""
303+
right = _wide_right()
304+
right.index = np.arange(len(right), dtype=float)
305+
left = right.iloc[:0][["id"]]
306+
307+
out = _lean_prefilter_right(left, right, "id", Engine.PANDAS)
308+
assert len(out) == 0, (
309+
f"shrink returned {len(out)} rows on a float index — bare [0:0] label-sliced "
310+
"instead of position-sliced, so the optimization did not fire"
311+
)
312+
pd.testing.assert_frame_equal(
313+
left.merge(out, on="id", how="left"), left.merge(right, on="id", how="left"))

0 commit comments

Comments
 (0)