[PERF] Selective bound selection for range joins - #1663
Draft
samukweku wants to merge 14 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.
_separate_conditions_based_on_op picked one <-type predicate for le_lt and one >-type predicate for ge_gt via simple first-encountered-wins, same class of bug #1641/#1658 fixed for same-direction predicates - just one level removed, and feeding both join_algorithm values ('default' and 'regions') since they both consume mapping["le_lt"]/["ge_gt"] from this one function. Adds _maybe_select_better_range_bounds in _get_indices_non_equi.py, called after null-stripping (needed since _sample_candidate_cost assumes null-free inputs) and before dispatch to either range-join algorithm. Reuses _le_ge_1_or_more._sample_candidate_cost directly - no new sampling logic, no new Rust. le_lt and ge_gt are independent selection problems, not a joint one: both range-join algorithms always process ge_gt first and le_lt second regardless of which candidate either slot holds, so there's no "which bound is primary" question to answer, just two separate #1641-shaped picks. Scoped to keep in ('first', 'last'), mirroring #1658's original scoping - keep='all' extension can follow the same #1657 pattern as a scoped follow-up if wanted. Benchmark (join_algorithm=default, bounded skew - a much-less-selective extra <-type candidate, not a domain-wide one which causes real O(n^2) blowup rather than a representative pathology): n broad-first narrow-first ratio 1,000 2.28ms 1.29ms 1.77x 3,000 6.12ms 1.64ms 3.73x 10,000 23.04ms 3.23ms 7.14x 30,000 86.22ms 7.57ms 11.39x 100,000 321.69ms 32.46ms 9.91x join_algorithm=regions shows the same shape, roughly 2.1x-7.4x over the same range. After this fix, both algorithms hold flat at ~0.97-1.10x from 1k to 100k rows.
samukweku
marked this pull request as draft
August 21, 2026 06:56
samukweku
added a commit
that referenced
this pull request
Aug 21, 2026
…ui joins _get_indices_equi.py (joins combining an == predicate with non-equi predicates) consumes the same mapping["le_or_ge"]/["le_lt"]/["ge_gt"] as the pure non-equi dispatch tree, but #1658's _select_anchor and #1663's _maybe_select_better_range_bounds never reached it - same class of pathology, different dispatch path. Adds _maybe_select_better_equi_predicates in _get_indices_equi.py, mirroring both fixes: single-anchor case for _equi_not_range_join.py (reuses _le_ge_1_or_more._sample_candidate_cost directly, same as #1663), two-bound case for _equi_range_join.py. Scoped to keep in ('first', 'last'), matching #1658/#1663. _equi_uniq_join.py needs no fix and is correctly left untouched: it flattens every non-equi predicate into one unordered post-filter set (dict.fromkeys(rest)) regardless of which mapping key it came from, since the == predicate already reduces to at most one candidate per left row via a hash indexer - there's no anchor/window narrowing to optimize there in the first place. Verified this by reading the file before assuming the issue's original 3-call-site framing was complete; confirmed reordering mapping upstream of it is harmless (order- insensitive) either way, so applying the fix before the whole dispatch (not just the two paths that need it) is safe and simpler than special- casing which downstream function gets called first. Benchmark (equi join on a ~20-group column, plus 2 non-equi predicates with one much less selective than the other): n broad-first selective-first ratio 1,000 2.14ms 2.00ms 1.07x 3,000 2.56ms 2.13ms 1.20x 10,000 6.80ms 2.81ms 2.42x 30,000 40.38ms 4.51ms 8.95x After this fix, flat at ~1.00-1.06x across the same range.
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
_separate_conditions_based_on_oppicked one<-type predicate forle_ltand one>-type predicate forge_gtvia simple first-encountered-wins - the same class of bug [PERF] Optimize conditional_join anchor predicate selection #1641/[PERF] Selectivity-aware anchor predicate selection for conditional_join #1658 fixed for same-direction predicates, just one level removed, and feeding bothjoin_algorithmvalues ('default'and'regions') since they both consumemapping["le_lt"]/["ge_gt"]from this one shared function._maybe_select_better_range_boundsin_get_indices_non_equi.py, called after null-stripping (_sample_candidate_costassumes null-free inputs - has to run after that step, not inside_separate_conditions_based_on_opitself, which runs before it) and before dispatch to either range-join algorithm._le_ge_1_or_more._sample_candidate_costdirectly - no new sampling logic, no new Rust.le_ltandge_gtturn out to be independent selection problems, not a joint one: both range-join algorithms always processge_gtfirst andle_ltsecond regardless of which candidate either slot holds - so there's no "which bound is primary" question to answer, just two separate [PERF] Optimize conditional_join anchor predicate selection #1641-shaped picks done independently.keep in ('first', 'last'), mirroring [PERF] Selectivity-aware anchor predicate selection for conditional_join #1658's original scoping.keep='all'extension can follow the same pattern [PERF] Extend selectivity-aware anchor predicate selection to keep='all' #1657 used, as a scoped follow-up, if wanted - not attempted here.ELI5
conditional_joinsupports range joins - two conditions together, likea > loanda < hi, that box a value into a range. If you give it more than one candidate for either side (say, two different<-type conditions that could both act as the upper bound), it always used whichever one you happened to type first to do the real narrowing, even if that one wasn't the useful one - the exact same mistake #1641/#1658 already fixed for simpler joins, just showing up again here in a different code path. This PR applies the same fix: pick the genuinely useful candidate for each bound, not just the first one typed, independently for the lower and upper bound.Benchmark
Two eligible
<-type candidates for the upper bound, one much less selective than the other (bounded skew - deliberately not a domain-wide "always true" bound, since that causes a real, less-representative O(n²) blowup rather than a realistic pathology):join_algorithm="default":join_algorithm="regions"shows the same shape, roughly 2.1x-7.4x over the same range (worse constant factor thandefault, consistent with #1660's finding thatregions' asymmetric two-field construction generally carries a bigger tax).After this fix, both algorithms hold flat at ~0.97-1.10x from 1k to 100k rows.
Test plan
pixi run pytest tests/functions/test_conditional_join.py -v -n auto- 288 passed, 1 skipped (full existing suite, unchanged)_maybe_select_better_range_boundspicks the genuinely selective candidate (direct unit test, not just output equality); order-invariance for both thele_lt-side and the symmetricge_gt-side, acrosskeep='first'/'last'and bothjoin_algorithmvalues; a mock-based guard proving the selection is never invoked forkeep='all'; a no-op check confirming single-candidate range joins (the overwhelmingly common case) are completely unaffected.pixi run pytest --doctest-modules janitor/functions/conditional_join.pypixi run lint🤖 Generated with Claude Code