[PERF] Extend selective anchor selection to keep='all' - #1662
Draft
samukweku wants to merge 15 commits into
Draft
Conversation
…irst/last The non-Numba multi-predicate conditional_join path always used the first supplied <,<=,>,>= predicate as the binary-search anchor, regardless of selectivity. A broad predicate picked as anchor over a selective one could be 2x-77x slower (issue's own repro, reproduced here as 2x/8.8x/57x at 1k/3k/10k rows). For keep='first'/'last', output is provably invariant to anchor choice: the match set doesn't depend on which predicate narrows the window first, and the Rust index builders do a true min/max reduction over the matching window rather than picking by scan order. So _select_anchor evaluates every le/ge candidate's window cost and picks the cheapest, with a stable tie-break to avoid changing behavior on already-good orderings. keep='all' is left untouched: which column right gets sorted by (i.e. the anchor) determines output row order for that mode, so anchor choice can't be safely reordered there without an observable behavior change. Filed as a separate follow-up: #1657. Also fixes an unrelated crash found while adding coverage: multi-predicate keep='last' joins with a single </<= window called index_starts_only_keep_last without its required counts argument. Issue #1641
- _le_ge_1_or_more.py: keep the empty-index array local to _get_indices, matching the convention in sibling files, instead of a shared module- level constant. - test_conditional_join.py: add order-invariance coverage across all 3! permutations of 3 le/ge predicates (not just 2), since _select_anchor picks among however many candidates are supplied. Prompted by an independent review of #1658 that stress-tested the core invariance claim (ties, nulls, 3-predicate cases, 360+ permutation comparisons) and found no correctness break, but flagged these two as worth cleaning up before merge.
…ction # Conflicts: # CHANGELOG.md # tests/functions/test_conditional_join.py
Add a Core Principle and a dated Learned Patterns entry per the user's request: every PR should get a fresh-context review pass (e.g. a subagent with no memory of how the change was built) before being treated as done, since implementer bias won't catch what an independent read will - as demonstrated on PR #1658 itself.
…nchor best[0] only meant "cost" because of where it was placed in a tuple literal two lines below - not readable without cross-referencing the construction site. Track best_cost/best_pos/best_result as separate named locals instead; same tie-break semantics (strict <, first-seen wins on ties). Also adds a one-line docstring to _get_indices, which never had one (pre-existing gap, not something this PR introduced) - it only tripped interrogate's 55% threshold now because this commit's diff is scoped to a single file instead of several.
_dual_non_equi.py (the region-number algorithm backing join_algorithm= "regions" and range joins) linked the paper by bare URL only. Expand to a full citation with DOI, and surface the same reference in conditional_join's public docstring so it's discoverable from the rendered API docs, not just a source comment. Prompted by tracing #1641/#1658 back to this paper while investigating whether "regions" mode was already immune to the anchor-ordering bug - it isn't (see #1660), but the paper is directly relevant prior art for both that and #1659.
Replace exact-cost anchor selection (evaluate every candidate's real binary search, pick the cheapest) with a fixed-size random sample per candidate. Correctness is unaffected either way: for keep='first'/'last', output is provably invariant to which predicate anchors (see #1641), so a suboptimal sample-based pick only costs performance, never output. This fixes a real regression the exact version had: evaluating every candidate meant its own cost scaled with input size, so on inputs where the first-supplied predicate was already optimal, the exact version was measurably slower than doing nothing - up to ~1.86x at 300k rows, ~3 seconds of pure waste at 10M rows. Sampling decouples the selection cost from input size entirely: benchmarked at 0.99-1.00x (no measurable regression) from 1M to 10M rows, versus 1.4-1.86x for the exact version, while still correctly detecting and fixing the original pathological case (broad-vs-selective predicate order). Also fixes two bugs found while reviewing an in-progress draft of this same idea: - The sort-permutation-based reorder path assumed right's index was unique and a contiguous RangeIndex; get_indexer raises InvalidIndexError on any duplicate index. Removing the exact-and-reuse design entirely (in favor of sampling to choose, then a single plain call to _evaluate_le_ge_candidate) sidesteps this class of bug rather than patching it - unreachable via the public API today, since conditional_join resets both frames' indices before this code runs, but not something worth relying on staying true. - A shared/module-level RNG would have made anchor choice - and therefore performance, though never output - silently vary across otherwise-identical calls to the same query. Each call now seeds its own RNG.
…hoice Prior tests (order-invariance, "determinism") only asserted output equality, which holds regardless of which candidate anchors by construction - they couldn't distinguish correct selectivity-driven selection from a coin flip. Fixed their docstrings to stop overclaiming what they verify, and add direct unit coverage that inspects _select_anchor's actual return value: - picks the genuinely selective candidate, not just "some" candidate - picks the same logical predicate regardless of argument order - picks the same candidate across repeated calls (real determinism, not just output equality) - _sample_candidate_cost computes the exact expected window for all four operators, against hand-computed values - documents, with a deterministic repro, the known limitation that a fixed 1024-row sample can miss a rare-but-decisive feature (~36% miss probability for a feature present in 0.1% of rows - 0.999**1024) - correctness is unaffected either way, only anchor- choice quality Also found and fixed a bug in the two new selectivity-sensitive tests themselves while writing them: the existing _dual_le_ge_frames helper (used by the output-invariance tests) doesn't guarantee any real selectivity skew between its "broad" and "selective" columns - both sides of each column are drawn from similar-scale ranges, so which one is actually more selective is close to a coin flip per seed. Added _skewed_broad_selective_frames, which deliberately shifts one column's range to guarantee a real skew, for the tests that need to assert *which* candidate wins.
Add an ELI5 comment on _SAMPLE_SIZE - it's a reasonable round default (big enough to reliably tell "broad" from "selective", small enough to cost nothing even at huge n), not a value tuned against a benchmark sweep.
…anchor Per adversarial review: cost < best_cost relies on cost never being NaN (a NaN comparison is always False, so it would silently pin candidate 0 regardless of later candidates - wrong anchor, never wrong output). Currently unreachable, since inputs are guaranteed non-null/non-empty by the time this runs, but worth flagging in case either guarantee moves.
Formalizes a pattern already used in practice (PR #1644, issues #1641/ #1660): for changes involving non-obvious algorithmic or performance reasoning, include a plain-language ELI5 section between the technical summary and any benchmark numbers. Deliberately scoped to "non-obvious changes only" rather than every PR/issue, to avoid templating trivial changes.
…nd rows Prompted by #1660: a ratio that looked like it might fade out below 30k rows instead held flat at ~1.8-1.9x all the way to 50M. Document extending Benchmark/Measured impact tables far enough to show whether an effect shrinks away or plateaus at scale.
…pot each Several rules were stated verbatim in 3-5 separate places - the same over-templated-file problem the project's own best-practices guidance warns about (rules get lost when the file is padded with restatements): - markdownlint: was in Core Principles, the Markdown Linting commands section, Anti-Patterns DON'T/DO, and a Learned Patterns entry. Kept Core Principles (the rule) + Markdown Linting (the how-to); dropped the rest. - pixi usage: was in Development Environment (full detail) and Anti-Patterns DON'T/DO (verbatim restatement). Kept Development Environment. - notebook conversion: was in Notebook Commands (with the CRITICAL warning) and Anti-Patterns DON'T/DO. Kept Notebook Commands. - Adversarial review: full rule in Core Principles, near-identical restatement in Learned Patterns (both added in the same session). Kept Core Principles. - "build docs in the docs environment": folded the Learned Patterns entry's useful "why" into Documentation Commands directly instead of living as a separate breadcrumb saying the same thing. Common Anti-Patterns to Avoid shrinks to only the items that add information not already stated in full elsewhere (DataFrame mutation, tests, docstrings). Net -55 lines, no content lost - just one home per rule instead of several.
_select_anchor (from #1658) previously only ran for keep='first'/'last', leaving keep='all' on the original first-supplied-predicate anchor. That turned out to be a real, severe, unbounded pathology - worse than the one #1641/#1658 fixed for keep='first'/'last': n broad-first selective-first ratio 1,000 1.86ms 0.85ms 2.18x 3,000 9.51ms 1.08ms 8.84x 10,000 99.29ms 1.85ms 53.66x 30,000 916.34ms 3.90ms 235.14x 100,000 10457.91ms 13.07ms 799.86x Unlike keep='first'/'last' (which plateaus around ~1.8-1.9x once the sampling-based fix is in place, per #1658/#1660's benchmarks), this grows without bound in the row counts tested - broad-first at 100k rows alone took over 10 seconds. After this change, ratio is flat at ~1.0-1.06x from 1k to 10M rows. Row content (which rows appear, their values) is provably invariant to anchor choice for every keep mode - only keep='all' row ORDER can be affected, and that was never documented or guaranteed. Verified this is a real, not just theoretical, trade-off: constructed a deterministic tie case (both candidates' sampled costs equal) where row order does differ between argument orders, and confirmed content stays identical regardless via the same sorted-comparison pattern the rest of this file already uses for keep='all' tests. Note on methodology: an earlier attempt to benchmark this concluded keep='all' had "no real pathology" - that conclusion was wrong, caused by a shell cwd reset silently pointing `pixi run python <script>` at the wrong worktree's environment between tool calls, not a property of the code. Re-verified with explicit per-call timing and output-length assertions in a single, carefully-scoped invocation before trusting the numbers above.
Per adversarial review of #1662: this comment predated the fix in this same PR and said anchor choice "must remain fixed (first-supplied)" for keep='all' - no longer true, and directly contradicted by the six keep='all' tests immediately below it.
samukweku
marked this pull request as draft
August 21, 2026 06:56
samukweku
force-pushed
the
issue-1641-anchor-selection
branch
from
August 26, 2026 01:58
5939326 to
142e493
Compare
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.
Summary
_select_anchor(added in [PERF] Selectivity-aware anchor predicate selection for conditional_join #1658 for issue [PERF] Optimize conditional_join anchor predicate selection #1641) only ran forkeep='first'/'last', leavingkeep='all'on the original first-supplied-predicate anchor. That turned out to be a real, severe, unbounded pathology, worse in growth than the one [PERF] Optimize conditional_join anchor predicate selection #1641/[PERF] Selectivity-aware anchor predicate selection for conditional_join #1658 fixed - see Benchmark below._get_indices(janitor/functions/_conditional_join/_le_ge_1_or_more.py): remove thekeep == "all"guard so_select_anchorruns whenever there are 2+ candidates, regardless ofkeep.keepmode (established in [PERF] Selectivity-aware anchor predicate selection for conditional_join #1658). Onlykeep='all'row order can be affected by anchor choice (which columnrightgets sorted by before scanning) - and that order was never documented or guaranteed to begin with, so trading it for correctness-preserving performance is a clean win, not a breaking change.ELI5
conditional_join's multi-predicate path picks one condition to do the heavy lifting (the "anchor") and checks the rest more simply within that narrowed-down set. #1658 fixed a bug where, forkeep='first'/'last', it always used whichever condition you happened to type first as the anchor - even if that one wasn't the useful one - which could make a query up to ~77x slower purely from argument order.That fix was scoped to
keep='first'/'last'only, because forkeep='all'the anchor also determines what order the results come back in, and changing that felt like it needed more care. Turns outkeep='all'had the same first-typed-wins bug the whole time, just never fixed - and it's worse: unlikekeep='first'/'last'(which tops out around ~1.8-2x slower even in the worst case),keep='all''s slowdown kept growing the bigger the data got, with no ceiling in the sizes tested - a query that took 13ms with a good predicate order took over 10 seconds with a bad one, at only 100k rows. This PR applies the same fix here: pick the actually-useful condition instead of just the first one typed.Benchmark
Broad-vs-selective scenario from #1641/#1658,
keep='all'this time, before this fix:Unlike
keep='first'/'last'(which plateaus around ~1.8-1.9x once fixed - see #1660's table for the same shape of measurement on a different code path), this grows without bound across every size tested - I didn't push further than 100k rows before-fix since a singlebroad-firstcall was already over 10 seconds there.After this fix, from 100k up to 10M rows:
Test plan
pixi run pytest tests/functions/test_conditional_join.py -v -n auto- 282 passed, 1 skipped (full existing suite, unchanged)_select_anchoris now actually invoked forkeep='all'(mock-based,wraps=the real implementation so it still runs); content-invariance across all 4 operators forkeep='all'(sorted comparison, matching this file's existingkeep='all'test pattern); a deterministic tie-case construction proving row order genuinely can differ between argument orders (while content never does) - built by finding a case where both candidates' sampled costs tie exactly, since that's the only condition under which this was found to actually happen (30 seeds of realistically-skewed data never produced a row-order difference - the sampling reliably picks the same logical predicate regardless of argument order when there's a real selectivity gap to detect).pixi run pytest --doctest-modules janitor/functions/conditional_join.pypixi run lint🤖 Generated with Claude Code