Skip to content

refactor: pack long Rust parameter lists into structs (#168) - #396

Merged
iceboundrock merged 3 commits into
mainfrom
autoforge/168
Nov 11, 2026
Merged

iceboundrock merged 3 commits into
mainfrom
autoforge/168

Conversation

@iceboundrock

@iceboundrock iceboundrock commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

#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 public LTSeq.asof_join signature is unchanged.

  • asof_join_impl takes its matching arguments as one AsofJoinOptions.
  • The parallel Parquet scan builds one RowGroupScan per 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_join returns a named RenamedRight instead of a (DataFrame, Vec<(String, String)>, Vec<String>) tuple, so the call site reads fields by name.

The internal asof_join binding changed shape

The 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 py is obtained. The issue's reason for keeping the binding flat was that it is the Python API, but it isn't: _inner.asof_join is called only from JoinMixin.asof_join, and the public API is that wrapper.

The maintainer chose to change the internal binding (recorded on #168). It now takes on as one (left, right) pair of time columns and by as a list of (left, right) column pairs: seven parameters including &self and py. Pairs make the left/right correspondence part of the argument's shape, so Python can no longer pass by lists of different lengths; the implementation's length check stays for Rust callers. joins.py and ltseq_core.pyi follow the new shape.

Validation

  • cargo clippy --offline --locked --all-targets -- -D warnings: passes, with no #[expect] left under src/.
  • cargo test: 191 passed (on the first commit; the second touches only the binding, which Rust tests don't call).
  • After maturin develop with the second commit: all 1234 pytest tests matching asof passed (test_asof_join.py, test_asof_by.py, test_asof_null_times.py and as-of cases elsewhere), plus test_no_materialization_rule.py and test_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

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
@iceboundrock

Copy link
Copy Markdown
Contributor Author

AI Code Review — Round 1

Reviewed HEAD: 8b399d5956a735f56774c1609b83e6afe9cb547f against base main (merge base d0b2ae8e9721d13dcb9e324fa10d452fa43c98b4)

Findings

R1-F1 [blocked] src/lib.rs:1005-1010 — Resolve the unmet fourth-suppression acceptance criterion

Required resolution:

Resolve the conflict with issue #168's explicit requirement to remove all four targeted lint expectations. Either remove the remaining binding-level expectation while preserving the specified flat Python signature and warning-free Clippy, or obtain an explicit maintainer-approved amendment permitting this binding exception and record that decision in the issue and PR. Merely rewriting the suppression's reason or proposing an exception in the PR description does not establish acceptance. Do not change the binding signature or weaken the lint policy without approval.

Spec

Partially satisfied. The implementation packages the as-of join arguments, groups the parallel row-group scan inputs, and names the join helper's return value. The Python binding signature and defaults remain unchanged. However, only three of the four targeted expectations are removed; the PR explicitly acknowledges retaining the fourth. No supplied comment records approval of this deviation. Full pytest success is not yet established by the supplied pending checks or this review.

Standards

The refactor follows the repository's borrowing, explicit-field initialization, named-struct, and thin-binding conventions. GIL release still encloses as-of execution, and no new materialization boundary or unsafe operation is introduced. No behavior change requiring changelog, docstring, or stub updates was identified. The remaining issue is the unresolved acceptance criterion, not the quality of the struct refactors.

Assessment

The code changes appear behavior-preserving: argument and result mappings are unchanged, and the borrowed scan context remains alive throughout the synchronous Rayon collection. No additional correctness or security defect was identified. The PR is not ready under the current specification because the binding exception requires an explicit resolution.

Observations

The PR description transparently identifies a genuine constraint conflict: retaining seven flat Python parameters plus a receiver exceeds Clippy's default argument limit even if the explicit Python token is eliminated. This explains the remaining expectation but does not authorize changing the acceptance criteria. The supplied PR head and base match this review's binding.

Verification

Read repository guidance, issue, PR description/comments, checks, follow-up context, the complete bound diff, and surrounding implementations and callers. cargo clippy --offline --locked --all-targets -- -D warnings passed. cargo test --offline --locked initially failed to locate the local Python shared library; rerunning with Python's configured library directory in LD_LIBRARY_PATH passed all 191 tests. git diff --check passed. An isolated CPython 3.14 release-wheel build exceeded the 120-second and subsequent 600-second limits, so pytest was not run. No build/test processes remained running afterward. All four supplied CI checks were pending. HEAD remained the bound revision and the worktree remained clean.

Summary

One blocking specification finding requires another resolution round. The implemented refactors show no identified behavioral regression, and local Clippy and Rust tests pass. Python-suite verification remains incomplete.

Needs another fix round: YES

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.
@iceboundrock

Copy link
Copy Markdown
Contributor Author

Maintainer decision on R1-F1 (recorded on #168): the internal _inner.asof_join binding may change shape; the public LTSeq.asof_join must not. 3d6025b takes on/by as (left, right) pairs and removes the last #[expect].

@iceboundrock

Copy link
Copy Markdown
Contributor Author

Maintainer decision on R1-F1 (recorded on #168): the internal _inner.asof_join binding may change shape; the public LTSeq.asof_join must not. 3d6025b takes on and by as (left, right) pairs and removes the last #[expect]. The PR description is updated to match.

@iceboundrock

Copy link
Copy Markdown
Contributor Author

AI Code Review — Round 2

Reviewed HEAD: 3d6025b9fd9e1159a662d9ddfe2cc220e1c8012f against base main (merge base d0b2ae8e9721d13dcb9e324fa10d452fa43c98b4)

Findings

R2-F1 [nit] src/ops/asof_join.rs:108-110 — Correct the binding signature description

Required resolution:

Update the AsofJoinOptions documentation to describe the current grouped on and by binding arguments rather than claiming the binding retains a flat Python signature.

Spec

The four targeted suppressions are removed, and the public LTSeq.asof_join signature remains unchanged. The issue records a maintainer decision approving the internal binding signature change. Full pytest completion is not established by the supplied checks.

Standards

The struct refactors follow the repository's thin-binding and GIL-release conventions. One new Rust documentation comment inaccurately describes the changed binding.

Assessment

The argument mappings and scan and join refactors appear behavior-preserving for the public API. The stale documentation requires correction; no additional correctness or security defect was identified.

Observations

R1-F1 no longer applies: the issue records the maintainer's approval of the changed internal binding, the PR records that decision, and the remaining suppression was removed. The supplied PR head and base match the review binding.

Verification

Inspected repository guidance, issue and maintainer comments, PR and prior review, checks, full bound diff, relevant implementations and callers. git diff --check and cargo clippy --offline --locked --all-targets -- -D warnings passed. The three as-of pytest files passed: 1189 tests. cargo test --offline --locked compiled but could not launch because the linked Python 3.15 shared library was unavailable. The supplied macOS and memory checks passed; both Linux build-and-test checks were pending. The worktree remained clean.

Summary

One documentation nit needs a fix round. Targeted Python tests and Clippy passed; full-suite CI was pending in the supplied checks.

Needs another fix round: YES

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.
@iceboundrock

Copy link
Copy Markdown
Contributor Author

AI Code Review — Round 3

Reviewed HEAD: 3e94fdcc70959b97978c7b69c044f2afe71eb10b against base main (merge base d0b2ae8e9721d13dcb9e324fa10d452fa43c98b4)

Findings

None.

Spec

The 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.

Standards

The refactors follow the repository's named-struct, thin-binding and GIL-release conventions. No standards violation was identified.

Assessment

The argument mappings and the join and parallel-scan refactors appear behavior-preserving. No correctness or security defect was identified.

Observations

R2-F1 is resolved: the options documentation now describes the grouped binding arguments. The supplied PR head and base match the review binding.

Verification

Inspected repository guidance, issue and maintainer decision, PR description and prior reviews, checks, the full bound diff, and related implementations. git diff --check and cargo clippy --offline --locked --all-targets -- -D warnings passed. The three as-of pytest files passed with 1189 tests using the existing local extension, whose binding has the new signature; the extension was not rebuilt during this review. cargo test --offline --locked compiled but could not launch because the linked Python 3.15 shared library was unavailable. All supplied CI checks were pending. The worktree remained clean.

Summary

No findings require another fix round. Targeted Python tests and Clippy passed; full-suite CI remains pending.

Needs another fix round: NO

@iceboundrock
iceboundrock merged commit e88d417 into main Nov 11, 2026
4 checks passed
@iceboundrock
iceboundrock deleted the autoforge/168 branch November 11, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: 长参数列表打包为参数 struct(asof_join / parallel_scan)

1 participant