From 5dfc4d66415d572c54ffc87135d8bd67f286c9aa Mon Sep 17 00:00:00 2001 From: samukweku Date: Fri, 21 Aug 2026 17:20:42 +1000 Subject: [PATCH 1/3] docs(agents): add inequality-joins paper as a conditional_join research reference Preserves an uncommitted edit from earlier session work (added when researching #1659) that hadn't landed on any branch yet. --- AGENTS.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 39f1d046e..4a7e59727 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -400,6 +400,24 @@ that looked like it might fade out below 30k rows instead held flat at ~1.8-1.9x all the way to 50M, which is the number that actually matters for deciding whether it's worth fixing. +### Conditional-Join Performance Research + +When investigating current or future `conditional_join` performance work, +include the region-number algorithms from Dathan and Trausan-Matu, +*Algorithms for Computing Inequality Joins* (DATA 2018), among the design +options considered: . + +The paper's two-comparison algorithm underlies the existing `regions` path. +Its Section 3.2 also describes a distinct multi-comparison strategy that +computes regions for every predicate and dynamically chooses the driving +field. Consider both the existing implementation and the paper's fuller +algorithm when researching predicate selection, field-role assignment, +relation orientation, candidate materialization, or other inequality-join +optimizations. Treat the paper as an option to evaluate, not an automatic +implementation mandate: preserve pyjanitor's ordering, dtype, null, index, +and extension-array behavior, and validate performance claims against +representative real inputs and the large-scale benchmark guidance above. + ### Code Style Rules - **Line length**: 88 characters (ruff default) @@ -580,6 +598,8 @@ pass, and removed from here to avoid restating the same rule twice. - **2026-08-21**: Added mandatory adversarial review before every PR - **2026-08-21**: Added PR/issue writing convention (ELI5 for non-obvious changes) - **2026-08-21**: Added convention to extend benchmarks to scale (10M-50M rows) +- **2026-08-21**: Added the inequality-joins paper as a design reference for + current and future `conditional_join` performance work - **2026-08-21**: Cleanup pass - removed 5x-duplicated markdownlint rule, 3x -duplicated pixi/notebook rules, and Learned Patterns entries already integrated into main sections From b90b8897334e8fd1e789b741affcf47557756bcb Mon Sep 17 00:00:00 2001 From: samukweku Date: Fri, 21 Aug 2026 17:20:51 +1000 Subject: [PATCH 2/3] perf(conditional_join): extend range-join bound selection to keep='all' Removes the keep in ('first', 'last') guard around #1659's selective le_lt/ge_gt bound selection for range joins, mirroring #1657's identical extension of #1658's same-direction anchor selection. Matched row content is unaffected either way; for keep='all' bound choice can still affect row order (never guaranteed), which the pre-existing pathology was unbounded rather than mild for, unlike the first/last case. Issue #1666. --- CHANGELOG.md | 1 + .../_get_indices_non_equi.py | 24 +++-- tests/functions/test_conditional_join.py | 89 +++++++++++++++++-- 3 files changed, 100 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2495389f7..3c0674d6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] +- [ENH] Range joins now also apply #1659's selective bound selection for `keep='all'`, not just `keep='first'`/`'last'` - the pathology was unbounded rather than merely mild for `keep='all'` on this path, so it was worth the same fix; matched row content is unaffected, only row order can change (never guaranteed to begin with). - Issue #1666 @samukweku - [ENH] Range joins (a `<`/`<=` predicate combined with a `>`/`>=` predicate) in `conditional_join` now pick the most selective candidate independently for each bound, when there are 2+ eligible predicates of either type and `keep` is `'first'` or `'last'` - fixing the same class of pathological slowdown from unfavorable predicate ordering as Issue #1641, for both `join_algorithm='default'` and `'regions'`. - Issue #1659 @samukweku - [ENH] `conditional_join` now picks the most selective `<`/`<=`/`>`/`>=` predicate as its binary-search anchor (instead of the first one supplied) when `keep` is `'first'` or `'last'`, fixing pathological slowdowns from unfavorable predicate ordering; the choice is estimated from a fixed-size sample so the selection cost no longer scales with input size; `keep='all'` output is unaffected. - Issue #1641 @samukweku - [BUG] Fix `conditional_join` crash for `keep='last'` joins with a single `<`/`<=` window and multiple non-equi conditions - a missing `counts` argument to the Rust `index_starts_only_keep_last` call. - Issue #1641 @samukweku diff --git a/janitor/functions/_conditional_join/_get_indices_non_equi.py b/janitor/functions/_conditional_join/_get_indices_non_equi.py index 160a48c99..c8a55a1af 100644 --- a/janitor/functions/_conditional_join/_get_indices_non_equi.py +++ b/janitor/functions/_conditional_join/_get_indices_non_equi.py @@ -43,11 +43,17 @@ def _maybe_select_better_range_bounds( both range-join algorithms always process `ge_gt` first and `le_lt` second regardless of which candidate either one is). - Mutates and returns `mapping`. The matched row set is unaffected - either way (see #1641's invariance proof, which this reuses); only - performance and, for `keep='all'`, row order can change - so this is - only worth calling when there's an actual choice to make and - `keep in ('first', 'last')`, mirroring #1658's own original scoping. + Mutates and returns `mapping`. The matched row set and its values are + unaffected either way, for every `keep` mode (see #1641's invariance + proof, which this reuses). For `keep='first'`/`'last'`, output is + invariant entirely. For `keep='all'`, bound choice can still affect + output row order (which column `right` gets sorted by) - never row + content - see issue #1666, which extended this to `keep='all'` on + the same basis #1657 already established for the same-direction + path: that row order was never documented or guaranteed here to + begin with, and the pre-#1666 pathology for `keep='all'` on this + path was severe (unbounded, not the mild ~2-9x `keep='first'`/`'last'` + saw) - see #1666 for the numbers. """ for bound_key, candidates_key in ( ("le_lt", "le_lt_candidates"), @@ -60,7 +66,11 @@ def _maybe_select_better_range_bounds( best = _select_range_bound(candidates, df, right) if best == current: continue - mapping["le_or_ge"] = [current, *[c for c in mapping["le_or_ge"] if c != best]] + # `best` is a le_or_ge member being promoted to `bound_key`; + # `current` demotes into its slot. Order within le_or_ge doesn't + # matter - it's always applied as an unordered set of independent + # post-filters - so an in-place swap is enough. + mapping["le_or_ge"][mapping["le_or_ge"].index(best)] = current mapping[bound_key] = best return mapping @@ -102,7 +112,7 @@ def _get_indices( "left_index": empty_array, "right_index": empty_array, } - if mapping["is_range_join"] and keep in ("first", "last"): + if mapping["is_range_join"]: mapping = _maybe_select_better_range_bounds(mapping=mapping, df=df, right=right) if not mapping["is_range_join"]: if (len(mapping["le_or_ge"]) == 1) or (join_algorithm == "default"): diff --git a/tests/functions/test_conditional_join.py b/tests/functions/test_conditional_join.py index b49170729..fde8ff3b4 100644 --- a/tests/functions/test_conditional_join.py +++ b/tests/functions/test_conditional_join.py @@ -4913,22 +4913,97 @@ def test_range_join_ge_gt_order_invariant(join_algorithm, keep): assert_frame_equal(bad_order, good_order) -def test_range_join_bound_selection_skipped_for_keep_all(): - """`_maybe_select_better_range_bounds` must never run for `keep='all'` - - mirrors #1658's original scoping (row order, not content, is what's - at stake there); #1657-style extension to `keep='all'` is out of - scope for this fix.""" +def test_range_join_bound_selection_used_for_keep_all(): + """`_maybe_select_better_range_bounds` must be invoked for `keep='all'` + too (issue #1666, extending #1659 the same way #1657 extended #1658 + for the same-direction case) - bound choice never changes matched row + content, only (for `keep='all'`) the order those rows come back in, + so there's no correctness reason to leave `keep='all'` on the + unselective first-supplied-bound path.""" df, right = _skewed_range_frames(seed=2, n=50) ge_gt = ("a", "lo", ">") narrow = ("a", "hi_narrow", "<") broad = ("a", "hi_broad", "<") + from janitor.functions._conditional_join import _get_indices_non_equi as m + with mock.patch( "janitor.functions._conditional_join._get_indices_non_equi." - "_maybe_select_better_range_bounds" + "_maybe_select_better_range_bounds", + wraps=m._maybe_select_better_range_bounds, ) as patched: df.conditional_join(right, ge_gt, broad, narrow, keep="all", how="inner") - patched.assert_not_called() + patched.assert_called_once() + + +@pytest.mark.parametrize("join_algorithm", ["default", "regions"]) +def test_range_join_le_lt_content_invariant_keep_all(join_algorithm): + """For `keep='all'`, the *set* of matched rows (and their values) must + not depend on which order the two `<`-type candidates are supplied + in, even though row order is no longer guaranteed to match between + orders (see #1666) - compare sorted, mirroring the pre-existing + `keep='all'` tests elsewhere in this file.""" + df, right = _skewed_range_frames(seed=4) + ge_gt = ("a", "lo", ">") + narrow = ("a", "hi_narrow", "<") + broad = ("a", "hi_broad", "<") + columns = ["a", "lo", "hi_narrow", "hi_broad"] + + bad_order = df.conditional_join( + right, + ge_gt, + broad, + narrow, + keep="all", + how="inner", + join_algorithm=join_algorithm, + ).sort_values(columns, ignore_index=True) + good_order = df.conditional_join( + right, + ge_gt, + narrow, + broad, + keep="all", + how="inner", + join_algorithm=join_algorithm, + ).sort_values(columns, ignore_index=True) + assert_frame_equal(bad_order, good_order) + + +def test_range_join_row_order_can_differ_for_keep_all_on_tie(): + """Documents the actual trade-off #1666 accepts, mirroring #1657 for + the same-direction case: for `keep='all'`, which candidate is picked + for `le_lt` is the secondary sort key `right` is ordered by whenever + `ge_gt`'s right column isn't already strictly sorted - so ties there + let bound choice affect row order. Verified empirically: this needs + an actual tie in the `ge_gt` column that also isn't already + monotonic (a constant `ge_gt` column, e.g., never triggers a real + sort at all, so never reproduces this) - once both hold, the two + argument orders produce the same content in a different order. + Content stays identical either way (see the content-invariance test + above); this test exists to make the trade-off visible, not to pin a + specific order.""" + df = pd.DataFrame({"a": [0]}) + right = pd.DataFrame( + { + "lo": [5, -100, -100, -100], + "hi_x": [999, 130, 110, 120], + "hi_y": [999, 101, 103, 102], + } + ) + ge_gt = ("a", "lo", ">") + cond_x = ("a", "hi_x", "<") + cond_y = ("a", "hi_y", "<") + + x_first = df.conditional_join(right, ge_gt, cond_x, cond_y, keep="all", how="inner") + y_first = df.conditional_join(right, ge_gt, cond_y, cond_x, keep="all", how="inner") + + columns = list(x_first.columns) + assert_frame_equal( + x_first.sort_values(columns, ignore_index=True), + y_first.sort_values(columns, ignore_index=True), + ) + assert not x_first.reset_index(drop=True).equals(y_first.reset_index(drop=True)) def test_range_join_bound_selection_noop_with_single_candidate(): From 5d73e4bca445502348ec0d763b3d087f4fece006 Mon Sep 17 00:00:00 2001 From: samukweku Date: Fri, 21 Aug 2026 17:23:08 +1000 Subject: [PATCH 3/3] docs(agents): scope conditional join guidance --- AGENTS.md | 20 +--- janitor/functions/_conditional_join/AGENTS.md | 103 ++++++++++++++++++ 2 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 janitor/functions/_conditional_join/AGENTS.md diff --git a/AGENTS.md b/AGENTS.md index 4a7e59727..a5eeed602 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -402,21 +402,9 @@ for deciding whether it's worth fixing. ### Conditional-Join Performance Research -When investigating current or future `conditional_join` performance work, -include the region-number algorithms from Dathan and Trausan-Matu, -*Algorithms for Computing Inequality Joins* (DATA 2018), among the design -options considered: . - -The paper's two-comparison algorithm underlies the existing `regions` path. -Its Section 3.2 also describes a distinct multi-comparison strategy that -computes regions for every predicate and dynamically chooses the driving -field. Consider both the existing implementation and the paper's fuller -algorithm when researching predicate selection, field-role assignment, -relation orientation, candidate materialization, or other inequality-join -optimizations. Treat the paper as an option to evaluate, not an automatic -implementation mandate: preserve pyjanitor's ordering, dtype, null, index, -and extension-array behavior, and validate performance claims against -representative real inputs and the large-scale benchmark guidance above. +Detailed guidance for `conditional_join` internals, behavioral invariants, +performance research, the inequality-joins paper, benchmarks, and verification +lives in `janitor/functions/_conditional_join/AGENTS.md`. ### Code Style Rules @@ -600,6 +588,8 @@ pass, and removed from here to avoid restating the same rule twice. - **2026-08-21**: Added convention to extend benchmarks to scale (10M-50M rows) - **2026-08-21**: Added the inequality-joins paper as a design reference for current and future `conditional_join` performance work +- **2026-08-21**: Moved detailed conditional-join guidance into the scoped + `_conditional_join/AGENTS.md` - **2026-08-21**: Cleanup pass - removed 5x-duplicated markdownlint rule, 3x -duplicated pixi/notebook rules, and Learned Patterns entries already integrated into main sections diff --git a/janitor/functions/_conditional_join/AGENTS.md b/janitor/functions/_conditional_join/AGENTS.md new file mode 100644 index 000000000..c0f6b70ad --- /dev/null +++ b/janitor/functions/_conditional_join/AGENTS.md @@ -0,0 +1,103 @@ +# Conditional Join Agent Guidance + +This file extends the repository-level `AGENTS.md` for work under +`janitor/functions/_conditional_join/`. + +## Behavioral Invariants + +- Preserve exact output values, row ordering, indexes, column structure, + dtypes, null semantics, and extension-array behavior. +- Preserve aggregation, `return_matching_indices`, and + `include_join_positions` behavior when changing index-generation internals. +- Treat `keep="all"` ordering as observable behavior. Changing the anchor or + sort key can change its row order even when the set of matches is unchanged. +- For `keep="first"` and `keep="last"`, verify that any reordered search path + still reduces matches using original right-row positions rather than scan + order. +- Do not assume caller indexes are unique or contiguous unless the public + dispatch path has normalized them before the relevant operation. Prefer + position-space internally when positions are the intended representation. +- Preserve stable sorting and the distinctions between `<`, `<=`, `>`, and + `>=`, especially around duplicates and nulls. + +## Architecture Map + +- `conditional_join.py` performs public validation, normalizes frames, and + dispatches to the internal algorithms. +- `_get_indices_non_equi.py` selects the maintained non-equality path. +- `_le_ge_1_or_more.py` handles one or more same-direction inequality + predicates for the default algorithm. +- `_dual_non_equi.py` implements the two-comparison region-number algorithm. +- `_not_range_join_regions.py` dispatches same-direction predicates for + `join_algorithm="regions"`. +- `_helpers.py` contains shared filtering and index-materialization logic; + changes there can affect multiple join modes. + +Follow dispatch and result materialization end to end before concluding that +an internal ordering or index representation is unobservable. + +## Performance Research + +When investigating current or future `conditional_join` performance work, +include the region-number algorithms from Dathan and Trausan-Matu, +*Algorithms for Computing Inequality Joins* (DATA 2018), among the design +options considered: . + +The paper's two-comparison algorithm underlies the existing `regions` path. +Section 3.2 describes a separate multi-comparison strategy that computes +regions for every predicate and dynamically chooses the driving field. +Consider both the current implementation and the paper's fuller algorithm +when researching: + +- predicate or anchor selection; +- ordered field-role assignment in the asymmetric regions algorithm; +- relation orientation when frame sizes differ; +- candidate generation, filtering, and materialization; +- range joins and joins with three or more inequality predicates. + +Treat the paper as an option to evaluate, not an automatic implementation +mandate. Account for correlations between predicates: individual selectivity +does not necessarily identify the best predicate pair. + +## Benchmark Expectations + +Performance work must compare equivalent joins with different predicate +orders and include, where relevant: + +- `<`, `<=`, `>`, and `>=`; +- `keep="all"`, `keep="first"`, and `keep="last"`; +- two predicates and three or more predicates; +- selective-first, broad-first, and similarly selective predicates; +- sorted and unsorted inputs; +- dense, sparse, zero-match, and full-match cases; +- narrow and wide frames; +- nullable and extension dtypes; +- small inputs, where estimator overhead dominates; +- large inputs, extending toward 10M-50M rows when memory and runtime permit. + +Report absolute timings as well as ratios. Measure already-optimal inputs to +detect regressions introduced by the optimizer itself. For sampled or +estimated selection, include skewed, correlated, periodic, and rare-tail data, +and state the estimator's expected failure modes. + +## Verification + +Run Python only through `pixi`, as required by the repository-level guidance. +The primary focused suite is: + +```bash +pixi run pytest tests/functions/test_conditional_join.py -v +``` + +During iteration, narrower selections are acceptable, but run the complete +focused file before completion. Add independent-oracle tests where practical; +cross-join-and-filter is suitable for small correctness fixtures. + +Tests for an optimizer must assert its decision directly, not only final output +invariance. Final output can remain identical even when the intended anchor, +predicate pair, or algorithmic branch was never selected. + +Before treating a pull request as complete, follow the root requirement for a +fresh-context adversarial review, with particular attention to ordering, +nullable data, duplicate values, index representations, and performance on +already-selective inputs.