Repository navigation
style: check the Google Rust style guide's rules with clippy, and follow them - #239
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Clippy checks and lint configuration .github/workflows/ci.yml, CONTRIBUTING.md, clippy.toml, python/ddxdb/clippy.toml, crates/*/src/lib.rs, crates/ddx-core/src/test_utils.rs, python/ddxdb/src/lib.rs, crates/ddx-ad/src/sql/tests.rs |
CI and local checks add a warnings-as-errors Clippy run for python/ddxdb. Crate lint settings and test allowances are updated, and SQL tests replace wildcard imports with explicit imports. |
Must-use API annotations crates/ddx-ad/src/*.rs, crates/ddx-ad/src/sql/*.rs, crates/ddx-core/src/*.rs, crates/ddx-datafusion/src/*.rs |
Functions and methods gain #[must_use] annotations. The sign constructor also gains a targeted Clippy allowance. |
Autodiff and SQL processing crates/ddx-ad/src/contraction.rs, crates/ddx-ad/src/forward.rs, crates/ddx-ad/src/transpose.rs, crates/ddx-ad/src/elementwise.rs, crates/ddx-ad/src/opt/prune.rs, crates/ddx-ad/src/program.rs, crates/ddx-ad/src/sql/{mod.rs,share.rs}, crates/ddx-ad/src/{tables.rs,tangent.rs} |
Union arms and grouping keys use named fields. The changes also update operator arity handling, borrowing, tangent alias selection, pruning, and relation combination. |
Poisoned-lock and empty-result handling crates/ddx-ad/src/sql/cache.rs, crates/ddx-datafusion/src/{ad.rs,compiled.rs}, python/ddxdb/src/lib.rs |
Poisoned locks no longer expose cached values for reuse. Python release returns an error for a poisoned lock, and DataFusion reports an internal error when SQL returns no frame. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Merge Risk: 
39cd1
-
No merge-blocking risk was identified in the lint or runtime changes.
Security architecture risk: 
39cd1
-
Public Rust callers may need adjustment, and poisoned caches now rebuild plans rather than recover cached contents. The inspected paths preserve existing session selection, plan validation, and cleanup ordering. Downstream integration and panic scenarios remain only partially verified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The inspected changes affect planning and temporary-table execution through the caller's DataFusion session or Python backend. No additional principal or broader execution authority was identified in these changed paths. Tenant isolation and the maximum authority granted by a host application are not established by this evidence.
Trust Boundaries and Controls
- observed — Both reused and rebuilt compiled plans pass through scan refresh and validation. Scans resolve tables through the supplied session state; compatibility checks include schema, partitioning, ordering, emission type, and boundedness, and other execution nodes have their state reset. These are execution-integrity checks, not independent authorization checks.
Resilience and Maintainability Implications
- observed — The Python release handle still takes its owned cleanup value exactly once and drops the mutex guard before calling the backend. A backend cleanup panic therefore cannot poison this mutex. Repetition after a cleanup attempt remains a no-op, including after an ordinary cleanup error; the PR does not introduce that recovery limitation.
-
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: the PR adds Clippy checks for Google Rust style guide rules and applies the resulting code changes. The changeset matches this: new lint attributes in lib.rs files, CI… Docstring Coverage Docstring coverage is 82.93% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: the PR adds Clippy checks for Google Rust style guide rules and applies the resulting code changes. The changeset matches this: new lint attributes in lib.rs files, CI and clippy.toml updates, #[must_use] annotations, and the related refactors. The title is 77 characters, which is longer than ideal but still clear.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 82.93% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 41 functions across 30 files. (4 skipped: 4 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
…low them Each library's root warns on the guide's rules that clippy can check: missing docs, unwrap, a borrow taken by value, a panic left out of the docs, a missing #[must_use], and glob imports. CI now runs clippy on ddxdb too. The fixes: - #[must_use] on 70 pure functions; - pattern matching in place of a length check and an unwrap; - borrows in place of by-value slices, maps and plans, and a moved emit in place of a copy; - named fields in place of tuples (a union column's arms, a contraction's groups, the program cache's entries); - no read of a poisoned mutex: the program cache and a compiled plan are built again, and ddxdb's release raises; - explicit imports in the SQL layer's tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CvszJhH8pn8H9G69wEPMU2
|
😈🧪 Adversarial tester I tested this PR at 161db46 and found no bugs. Differential fuzz against main (a223f33). I ran the same seeds on ddxdb built from each, on DuckDB and DataFusion:
Both runs' logs and failure lists are byte-for-byte identical. The failures they share are the pre-existing #235–#238. ddxdb tests: 127 passed. Soak, 15 min: one failure in 1,158 cases, seed 239000365. It's a DataFusion Code read: the behavior changes are equivalent:
The poisoned-lock paths in 🤖 Generated with Claude Code |
…xdb 0.4.6 (#233) grad and vjp differentiate through UNION ALL, as jvp already did (#229). A loss joining one-row aggregates of constant data is again a scalar loss (#230). A step that one later step reads once is computed inside it: xarray-sql's GPT step has 64 steps rather than 77, about 1.5% faster on DuckDB (#231). The code follows the Google Rust style guide (#234, #239, #240), with instruction counts within 0.05% of before; a lock a panic poisoned is no longer read. Nothing public breaks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CvszJhH8pn8H9G69wEPMU2
Why
This is the second of three PRs that bring ddx in line with the Google Rust style guide, after #234 (formatting). It covers the guide's rules that clippy can check, plus a few clippy can't. Most of the code already followed them:
unwraps;unsafe.How
Lints. Each library's root (
ddx-core,ddx-ad,ddx-datafusion, ddxdb) warns on these. CI treats warnings as errors:missing_docsexpectoverunwrapclippy::unwrap_used(tests may unwrap:clippy.toml)clippy::needless_pass_by_valueclippy::missing_panics_doc#[must_use]on functionsclippy::must_use_candidate,clippy::return_self_not_must_useclippy::wildcard_importsddxdb leaves out
missing_docs, andneedless_pass_by_valuetoo, since pyo3 passes functions owned values. CI now runs clippy on ddxdb, which it never did.Fixes.
#[must_use]on 70 pure functions and builder methods.ddx_datafusion::ad::sqlreturns an error instead of panicking.stitchborrows its pieces, the dual'semittedborrows its tangents, andbroadcastborrows its columns;prune::finishmoves its emit instead of cloning it;SeedisCopy.Def::Union's arms areArm { branch, column };Group { side, column };Kept { key, program }.Rules not applied
emit::select(...)). ddx-ad's plan builders (emit,expr) read as a small language:call(add, vec![field(0), lit_f64(1.0)]). Prefixing hundreds of calls would add noise, and the guide's reason is Google's large-scale-change tooling.Option<&T>instead of&Option<T>(clippy::ref_option). The three hits take a prost child field (&Option<Box<Rel>>) as prost declares it. TakingOption<&Rel>would only lengthen 18 call sites.#[non_exhaustive]on every public enum and struct. The guide exempts types whose users the authors will fix, and ddx has no outside users. ddx-ad's errors, programs, tables and results already have it.Rcin the region tree, where regions share what is beneath them, andcatch_unwindinad_sql_text.rs's fuzzer, which reports a panic with its seed. Both are deliberate.async.Performance
Measured against main (a223f33) on xarray-sql's GPT step (
examples/prepared_step.rs, prepared, DataFusion) andad_perf'snnfamily, release builds run alternately.ad_perfnn process, user-space instructions (3 each)Instruction counts are the measure here. Another job was compiling on the machine (load 4–10), so wall time varied ±30% between runs of the same binary. The CPU time of six runs of main alone spread by 87%. The format-only build (#234) measured +3.6% in wall time against main in the same conditions, with identical instruction counts.
Next
Moving code out of non-leaf modules (
to_sql/mod.rs,sql/mod.rs).🤖 Generated with Claude Code
https://claude.ai/code/session_01CvszJhH8pn8H9G69wEPMU2
Summary by CodeRabbit
Bug Fixes
Chores