Repository navigation
refactor: pack long Rust parameter lists into structs (#168) - #396
Conversation
asof_join_impl takes its matching arguments as AsofJoinOptions, the parallel Parquet scan lends each row group task one RowGroupScan, and rename_right_df_for_join returns a named RenamedRight instead of a three-tuple. This removes three of the four lint suppressions #150 left as deferred debt. The PyO3 asof_join binding keeps its #[expect(too_many_arguments)]: its seven Python parameters plus the receiver are eight, one over clippy's limit, and the issue keeps that flat signature. Its reason now records that this is permanent rather than deferred. No behavior change. Refs #168
AI Code Review — Round 1Reviewed HEAD: FindingsR1-F1 [blocked]
|
The internal _inner.asof_join binding now takes the time columns as one (left, right) pair and the by= columns as a list of (left, right) pairs. That brings it to seven parameters with the receiver, so the last too_many_arguments suppression goes. The public LTSeq.asof_join is unchanged.
AI Code Review — Round 2Reviewed HEAD: FindingsR2-F1 [nit]
|
The AsofJoinOptions comment still said the binding kept Python's flat signature; it now takes on as a (left, right) pair and by as a list of pairs, which it unpacks into the per-side fields.
AI Code Review — Round 3Reviewed HEAD: FindingsNone. SpecThe four targeted suppressions are removed, and the public as-of join signature remains unchanged. The issue records maintainer approval for changing the internal binding. Full-suite success remains subject to CI. StandardsThe refactors follow the repository's named-struct, thin-binding and GIL-release conventions. No standards violation was identified. AssessmentThe argument mappings and the join and parallel-scan refactors appear behavior-preserving. No correctness or security defect was identified. ObservationsR2-F1 is resolved: the options documentation now describes the grouped binding arguments. The supplied PR head and base match the review binding. VerificationInspected repository guidance, issue and maintainer decision, PR description and prior reviews, checks, the full bound diff, and related implementations. SummaryNo findings require another fix round. Targeted Python tests and Clippy passed; full-suite CI remains pending. Needs another fix round: NO |
#150 brought clippy to zero warnings partly by suppressing four lints with
#[expect], recorded as deferred debt. The coding standard (docs/rust-coding-std.md§3.4) says long parameter lists should be grouped into a struct. This PR removes all four suppressions. It does not change behaviour, and the publicLTSeq.asof_joinsignature is unchanged.asof_join_impltakes its matching arguments as oneAsofJoinOptions.RowGroupScanper file, holding the path, projection, partition column, predicates, fast-path plan and column index. It lends that to each rayon task, which adds only its row-group index.rename_right_df_for_joinreturns a namedRenamedRightinstead of a(DataFrame, Vec<(String, String)>, Vec<String>)tuple, so the call site reads fields by name.The internal
asof_joinbinding changed shapeThe issue asked for two things that can't both hold: remove all four suppressions, and keep the PyO3 binding's flat seven-parameter signature. Clippy counts the receiver, so seven flat parameters are eight against its limit of seven, however
pyis obtained. The issue's reason for keeping the binding flat was that it is the Python API, but it isn't:_inner.asof_joinis called only fromJoinMixin.asof_join, and the public API is that wrapper.The maintainer chose to change the internal binding (recorded on #168). It now takes
onas one(left, right)pair of time columns andbyas a list of(left, right)column pairs: seven parameters including&selfandpy. Pairs make the left/right correspondence part of the argument's shape, so Python can no longer passbylists of different lengths; the implementation's length check stays for Rust callers.joins.pyandltseq_core.pyifollow the new shape.Validation
cargo clippy --offline --locked --all-targets -- -D warnings: passes, with no#[expect]left undersrc/.cargo test: 191 passed (on the first commit; the second touches only the binding, which Rust tests don't call).maturin developwith the second commit: all 1234 pytest tests matchingasofpassed (test_asof_join.py,test_asof_by.py,test_asof_null_times.pyand as-of cases elsewhere), plustest_no_materialization_rule.pyandtest_gil_release.py. The first commit's run also covered the join, semi/anti join, search-pattern and parallel Parquet scan files (2526 passed). I didn't run the full suite locally; CI runs it.Closes #168