Repository navigation
chore: allow clippy mutable_key_type on Expr hash sets in PushDownFilter (#26181) - #26189
patrickswedish wants to merge 1 commit into
Conversation
…ter (apache#26181) Running Clippy with all features enabled failed with clippy::mutable_key_type in PushDownFilter because HashSet<&Expr> in window partition key extraction and predicate checking uses Expr as a hash key. Expr transitively contains allocator/tracking abstractions with interior mutability. Annotate extract_partition_keys, potential_partition_keys, and reads_only_partition_keys with #[allow(clippy::allow_attributes, clippy::mutable_key_type)], matching the standard convention across DataFusion. Closes apache#26181. Signed-off-by: Patrick Ribbsaeter <233187023+patrickswedish@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26189 +/- ##
==========================================
- Coverage 82.79% 82.79% -0.01%
==========================================
Files 1149 1149
Lines 451645 451645
Branches 451645 451645
==========================================
- Hits 373940 373932 -8
- Misses 54929 54933 +4
- Partials 22776 22780 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Why don't we see this error in CI? |
|
Hi @alamb! The reason CI doesn't catch this is that CLIPPY_FEATURES="avro,integration-tests,extended_tests"
cargo clippy --all-targets --workspace --features "$CLIPPY_FEATURES" -- -D warningsHowever, DataFusion's contributor guide ( cargo clippy --all-targets --all-features -- -D warningsWhen This matches the existing allowances already in place across Note: I noticed that commit |
Can you please make a targeted PR to adjust the clippy rules, with a brief summary if the rationale as a follow on PR rather than reusing this one? Thank you |
|
@patrickswedish I got some sort of legal notice from your email which is not accurate and this I will flag your github account for abuse. |
…ache#26590) ## Which issue does this PR close? Closes apache#26181. ## Rationale for this change On `main` at `2717e25625ff0c51c1805f8df52d317979db74ec`, `cargo clippy --all-targets --all-features -- -D warnings` fails with two `clippy::mutable_key_type` errors in `alias_aggregate_exprs` and `rewrite_select_aggs`. The diagnostics follow `Expr` through nested `ScalarValue` arrays to Arrow's `TrackedReservation`. That mutable memory accounting does not affect expression equality or hashing. The earlier `PushDownFilter` occurrences discussed in apache#26189 are already fixed on `main`. ## What changes are included in this PR? Add explained lint allowances on the aggregate rewrite map declaration and `rewrite_select_aggs`, following existing `Expr` key handling. No runtime code changes. ## What is the testing strategy for this PR? - Reproduced both errors on unmodified `main`; the exact command passes with this change. - `cargo fmt --all` and `git diff --check` pass. - `uv run ./dev/rust_lint.sh` passes, including CI Clippy, dependency checks, and documentation builds. - All 240 existing DataFrame integration tests pass, including `test_dataframe_api_select_semantics`, aggregate aliases, and name collisions. - Extended workspace validation passes: 12,819 Rust tests passed, 8 ignored, and 538 SQL logic test files completed: ```sh RUST_BACKTRACE=1 cargo test --profile ci \ --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli \ --workspace --lib --tests --bins \ --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption ``` No new runtime tests or benchmarks: this change adds only lint attributes and explanatory comments. ## Are there any user-facing changes? The documented all-features Clippy command passes again. No SQL behavior or public API changes.
## Which issue does this PR close? Related to apache#26181 and the [CI follow-up requested in apache#26189](apache#26189 (comment)). ## Rationale for this change The existing Clippy feature set missed the Arrow buffer-pool lint failures exposed by `--all-features`. Run both configurations through the shared script so CI, local developers, and agents get the same checks. Keep the existing feature set: `force_hash_collisions` excludes normal hashing code when all features are enabled. ## What changes are included in this PR? - Run the existing feature-specific check followed by the all-features check in `ci/scripts/rust_clippy.sh`. - Apply `--write` and `--allow-dirty` consistently to both passes, retaining the clean-worktree guard and failure propagation. - Install CMake before invoking the script in CI, because the all-features build includes `snmalloc-sys`. ## What is the testing strategy for this PR? - `cargo fmt --all` and `git diff --check` pass. - Both Clippy passes in `ci/scripts/rust_clippy.sh` pass, as does `cargo clippy --all-targets --all-features -- -D warnings`. - The full `uv run ./dev/rust_lint.sh` suite passes, including the workflow validators and 28 workflow tests. - Ten shell behavior probes cover both configurations, write mode, the dirty-worktree guard, and failure propagation. A disposable Rust crate also verifies real fixes in both feature configurations, rejection of an initially dirty worktree, and `--allow-dirty` support. - The required extended workspace test command passes: 12,899 Rust tests and all 541 SQL logic test files; eight Rust tests are ignored. All validation above was run locally. Hosted CI must rerun for the updated head. ## Are there any user-facing changes? Local `rust_clippy.sh` and `dev/rust_lint.sh` runs now check both feature configurations. The all-features build requires CMake to be installed locally. No SQL behavior or public API changes.
Which issue does this PR close?
Closes #26181.
Rationale for this change
Running
cargo clippy --all-targets --all-features -- -D warningsfails indatafusion-optimizerwith threeclippy::mutable_key_typeerrors indatafusion/optimizer/src/push_down_filter.rs.This happens because
extract_partition_keys,potential_partition_keys, andreads_only_partition_keyscollect and check partition keys usingHashSet<&Expr>. BecauseExprtransitively containsScalarValuewith buffer allocations, Clippy flags&Expras possessing interior mutability.Throughout DataFusion (
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs,datafusion/optimizer/src/analyzer/resolve_grouping_function.rs,datafusion/expr/src/logical_plan/plan.rs, etc.), the standard resolution is to annotate intentionalExprhash keys with#[allow(clippy::allow_attributes, clippy::mutable_key_type)].What changes are included in this PR?
extract_partition_keys,potential_partition_keys, andreads_only_partition_keyswith#[allow(clippy::allow_attributes, clippy::mutable_key_type)]indatafusion/optimizer/src/push_down_filter.rs.What is the testing strategy for this PR?
cargo check -p datafusion-optimizer --all-targets --all-featurespasses cleanly without warnings.cargo test -p datafusion-optimizer --libpasses all 932 unit tests.rustfmt --check datafusion/optimizer/src/push_down_filter.rsverified clean formatting.Are there any user-facing changes?
None.