[MAINT] Add direct correctness tests and benchmarks by samukweku · Pull Request #28 · pyjanitor-devs/janitor-rs

samukweku · 2026-08-22T04:34:34Z

Summary

Extract representative kernels into plain-Rust *_core functions (ArrayView1 in, Array1 out, no PyO3 types) so they can be tested and benchmarked without a Python interpreter or pyjanitor checkout: binary_search_lt_core, compare_start_end_core, repeat_index_core/trim_index_core, and the three forward range-sum cores. Existing #[pyfunction] entry points remain thin wrappers.
Preserve integer cast-on-access behavior: typed NumPy views flow into the shared loop and only values inside requested ranges are widened. Focused conversion-count tests cover tiny prefix, suffix, and interval queries.
Make the established -1 "no match" sentinel contribute zero before any signed bound is cast to usize. Integer and float range-sum paths now share the same validation contract; comparison ranges use the same pre-cast rule.
Add 51 #[cfg(test)] unit tests covering empty arrays, zero matches, duplicate values, boundary positions, sentinel ranges, null masks, dtype behavior, and explicit integer wraparound.
Add a Criterion harness at 100 and 100,000 rows. Alongside throughput cases, the sum group has a single width-eight u32 query over a large column to catch accidental whole-column conversion on sparse workloads.
Add an rlib only for Rust test/benchmark targets. lib.rs keeps implementation trees private and exposes only eight benchmark targets through a hidden bench_support facade instead of publishing hundreds of internal wrappers as a Rust API.
Fix the eight concrete Clippy warnings in the pre-existing baseline and add a narrowly scoped too-many-arguments-threshold = 10 for intentional PyO3 entry-point arity.
Add CI for formatting, unit tests, Clippy with warnings denied, and benchmark compilation.
Add README.md with build/test/bench/lint commands, the PyO3 feature split, the macOS framework-Python loader workaround, downstream parity-test context, and the benchmarking process for Python/Rust boundary changes.
Add AGENTS.md with the extraction pattern and durable lessons from implementation and adversarial review, including sentinel validation across dtype-specific loops, sparse benchmark shape, explicit overflow semantics, and narrow benchmark exports.

This is test/benchmark foundation, not a full-crate kernel sweep. Overlapping kernel PRs should extend these cores and tests as they land.

Test plan

cargo test --no-default-features — 51/51 pass
cargo clippy --all-targets --all-features -- -D warnings — clean
cargo fmt --check — clean
cargo bench --no-default-features --no-run — compiles
Sparse u32 sum benchmark ran locally at both sizes; the width-eight query remained ~17 ns for 100- and 100,000-element columns
Full benchmark suite ran locally with all four kernel-family groups completing without panics
cargo doc --no-default-features --no-deps confirms the implementation trees are no longer emitted as public Rust API documentation
markdownlint AGENTS.md README.md — clean apart from the three documented long-line exceptions

Closes #21

🤖 Generated with Claude Code

https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

Adds a foundation for testing/benchmarking janitor-rs kernels directly, without a Python interpreter or pyjanitor checkout: one or two representative kernels per family (binary search, comparison, index building, aggregation) are extracted into plain-Rust `*_core` functions (ArrayView1 in, Array1 out, no PyO3 types), covered by #[cfg(test)] unit tests and a criterion benchmark harness in benches/kernels.rs. - Extract binary_search_lt_core, compare_start_end_core, repeat_index_core/trim_index_core, and sum_start_core/sum_end_core/ sum_start_end_core; the existing #[pyfunction] macros become thin wrappers around them. - Fix the 8 concrete clippy warnings in the pre-existing baseline (6 useless_conversion, 2 unnecessary_cast); add clippy.toml raising too-many-arguments-threshold to 10 for the ~240 macro-expanded too_many_arguments reports from intentional PyO3 entry-point arity. - Add .github/workflows/ci.yml running cargo fmt/test/clippy/bench --no-run on every push and PR. - Add README.md (test/bench/lint commands, why the extension-module feature needs --no-default-features for cargo test/bench, and how these tests relate to pyjanitor's own downstream parity tests) and AGENTS.md (build/test patterns and the non-obvious gotchas hit while wiring this up, for future agents/contributors). Issue #21 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

CI's dtolnay/rust-toolchain@stable pulled clippy 1.98.0, which flags three more instances of the same into_iter().zip(x.into_iter()) pattern in index_builder.rs (in functions this PR doesn't otherwise touch) that my local clippy 1.93.1 didn't catch. Same mechanical fix as the other 8. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

- benches/kernels.rs: bin_search fixture's `i % (n + 1)` was a no-op (i already ranges over 0..n), simplify to a plain range and comment it. - .github/workflows/ci.yml: drop --all-targets from the test step -- it compiled benches/kernels.rs under the test profile just to throw it away, since the separate `cargo bench --no-run` step already compiles it under the bench profile it actually ships with. - Add ELI5 doc comments to the three benchmark functions that lacked one, and to the harness=false rationale in Cargo.toml. From an independent code review of PR #28 (8 agent angles + a manual pass): no correctness bugs found across the extraction, casts, or the 44 new tests. These are the two cheap, safe findings worth fixing now; the other three (sum_*_core_with_cast's three near-identical loops, clippy.toml's threshold bump being crate-wide rather than macro-scoped, and only 4 of ~15 kernel families being extracted so far) are real but either touch code from in-flight parallel work on this branch or are already disclosed as intentional scope in the PR description, so noted rather than acted on here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

sum_end_core, sum_start_end_core, and compare_start_end_core cast start/end to usize unconditionally. A -1 sentinel (the crate's established "invalid/no match" convention, already guarded in binary_search_lt_core) casts to usize::MAX, and the inner loop then walks straight past arr's/matches' bounds instead of contributing 0 - contradicting sum_start_end_core's own new doc comment, which claimed this was already handled by a post-cast start_ >= end_ check. It isn't: a lone -1 end wraps to a value larger than any real start, so that check never fires. sum_start_core is unaffected: only `start` is user-controlled there (`end_` is always arr.len()), and a wrapped start_ naturally exceeds that fixed end_, giving the intended empty range with no crash risk. Also documents (without changing) the existing uint64 `value as i64` cast: it's a bit-reinterpretation for values >= 2^63, matching NumPy's own unsafe-cast semantics on the pyjanitor side of this boundary. Adds a locking-in test now that the shared *_with_cast helper is directly testable, closing a gap the new cast-tracking tests left (they only exercised the safe i32-to-i64 case). Found via adversarial code review of #28.

bin_search_lt used a shared Fixture struct; compare_start_end and sum_kernels built their arrays as loose inline lets instead - an inconsistent pattern within one file with no guidance for which approach a future benchmark should follow. Renames Fixture -> BinarySearchFixture and adds CompareFixture, IndexBuilderFixture, and SumFixture so every benchmark follows the same <Kernel>Fixture::new(n) convention. Not a single shared fixture: each kernel genuinely needs a different input shape (documented on each struct), and forcing them together would just bloat one struct with fields only one kernel uses - the exact anti-pattern this was meant to avoid. No behavior change: same arrays, same values, same benchmarks. Verified via `cargo bench --test` (all groups run) plus fmt/clippy/test. Found via adversarial code review of #28.

… ELI5 Adds ELI5-labeled explanations for why the -1 sentinel must be checked before, not after, casting to usize (sum_ends, sum_starts_ends, comp), and for why the u64-to-i64 cast can flip sign past 2^63 (sum_starts) - this PR's fixes had the technical "what" but not the plain-language "why" the ELI5 convention calls for. AGENTS.md: promotes "ELI5 code comments" from a line buried inside "Adding a new dtype-generic kernel" (framed as new-kernel-only) to its own Core Principle covering fixes/guards/refactors too, since the -1 guard fix is exactly the kind of change that convention should have already applied to. The old section now references the principle instead of restating it. Adds two Learned Patterns entries: the -1 sentinel/cast-ordering bug itself (a real, generalizable gotcha for future *_core extraction/review), and the ELI5 generalization.

…oaded Add two durable policy points to the Agent Constitution/Core Principles, prompted by the first review of PR #28 (8 agent angles + a manual pass) reporting "no correctness bugs found" while missing a real one: three *_core functions cast a -1 sentinel to usize unconditionally and would walk off the array, caught only by a later adversarial pass that explicitly tried the sentinel value. - Review every PR adversarially: try -1/sentinel values, 0, exact boundaries, and one-past-"obviously safe" for every _core input, not just whether the code looks self-consistent. - Before reviewing any PR here, read this file in full first, and make sure any subagent/forked reviewer has it in context too -- a reviewer blind to this crate's sentinel convention is blind to exactly the bug class above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

compute_min_start_*/compute_max_start_* read arr[start_] unconditionally before checking whether start_ is a valid index -- if start == arr.len() (an empty search range) or start == -1 (the crate's "no match" sentinel, cast to usize::MAX), this panics instead of returning -1. The same root cause -- min/max need a real array element to seed their running comparison, unlike sum (0) or prod (1), which have a neutral identity -- turns out to affect all 14 forward min/max kernel files, not just the 2 named in the issue: starts, ends, starts_ends, positions, starts_matches, ends_matches, and starts_ends_matches, for both min and max. Confirmed via grep across the whole aggs tree that prod/prod_rev (identity element, no seed read) and min_rev/max_rev (seed from the row's own already-valid value via HashMap, not an indexed read) don't share this bug class. Each file gets the same shape of guard, placed before the unconditional read (and, for the *_matches variants, also before the count==0 branch's end_ - start_ subtraction, which underflows the same way): - starts/starts_ends/starts_matches/starts_ends_matches: start_ >= end_ - ends/positions/ends_matches: arr.is_empty() Extracted each into a testable pub fn <name>_core(...) (ArrayView1 in, no PyO3 types), matching the pattern established in PR #28/issue #21, with tests reproducing the exact panic from issue #27's repro plus the analogous empty-array panic in the *_end* variants that issue #27's own text incorrectly claimed were fine. 87 new/changed tests total. Noted but not fixed here (separate, unrelated bug, out of scope): compute_min_positions_int8/compute_max_positions_int8 are instantiated with type i64 instead of i8 -- filed as a follow-up issue. Issue #27 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

All 16 files under src/compare/ carried their own private copy of binary_compare, an i8/i64 numeric-code match for `>`, `>=`, `<`, `<=`, `==`, `!=`. An unrecognized code silently fell through to `!=` instead of being rejected, and every file re-decoded the operator on every single candidate pair inside the innermost loop instead of once per call. Adds src/compare/op.rs: a shared CompareOp enum with try_from_code (validates 0..=5, raises a clear PyValueError otherwise, mirroring aggs::ensure_tape_width's existing error pattern) and comparator() (picks the comparison once as a plain function pointer, so the per-candidate loop calls it directly instead of re-matching on the operator every time). Every compare file now decodes with CompareOp::try_from_code(op)? once near the top of the function, before the loop, and returns PyResult instead of a bare tuple so the validation error can propagate. Public Python export names and signatures are otherwise unchanged -- op stays i8 in 14 files and i64 in the 2 comp_no_range* files, and valid codes behave identically to before. comp.rs's existing compare_start_end_core (extracted in #21/#28) now takes CompareOp directly instead of a raw op code; its tests and benches/kernels.rs::bench_compare_start_end updated accordingly, and bench_support re-exports CompareOp. Addresses the "shared, validated comparison operator" opportunity from issue #24 -- the one-pass *_first output (#46) and the contiguous fast-path are the issue's other two opportunities, tracked separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3KAbkp6JV96KYXNc6EJmN

samukweku and others added 2 commits August 22, 2026 14:34

samukweku self-assigned this Aug 22, 2026

samukweku and others added 3 commits August 22, 2026 17:27

perf: preserve sparse integer range sums

eb21b3e

test: scope integer overflow semantics

46e93c4

This was referenced Aug 22, 2026

[PERF] Improve retained binary-search and comparison kernels #24

Open

[MAINT] Harden and simplify conditional-join index builders #25

Open

This was referenced Aug 22, 2026

[PERF] Add adaptive integer range-sum kernels #26

Open

[REFACTOR] Modularize PyO3 function registration #22

Closed

samukweku and others added 4 commits August 22, 2026 18:42

fix: close adversarial review gaps

af505cb

samukweku merged commit c4bc95b into main Aug 22, 2026
17 checks passed

samukweku mentioned this pull request Aug 22, 2026

[BUG] Guard unconditional seed reads in forward min/max kernels #29

Merged

5 tasks

This was referenced Aug 24, 2026

perf: one-pass output for binary_search_*_first kernels #46

Merged

[MAINT] Scope the too_many_arguments clippy allow to macro-generated pyfunctions only #47

Open

refactor: shared, validated comparison operator across compare kernels #51

Merged

Navigation Menu

Uh oh!

[MAINT] Add direct correctness tests and benchmarks - #28

[MAINT] Add direct correctness tests and benchmarks#28
samukweku merged 10 commits into
mainfrom
issue-21-test-bench-foundation

samukweku commented Aug 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

samukweku commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

samukweku commented Aug 22, 2026 •

edited

Loading