Follow Google's Rust style guide in file names, imports and names by alxmrs · Pull Request #55 · xqlsystems/einfold

alxmrs · 2026-10-26T20:23:14Z

🪭 Claude (einfold)

Stacked on #54. The second of two pull requests that bring einfold's Rust in line with Google's Rust style guide, as the maintainer asked. Everything here is mechanical, with no change in behavior.

Files named after their primary items. In einfold-datafusion:
- fold_node.rs holds FoldNode, detect_folds.rs holds DetectFolds, and ein_fold_exec.rs holds EinFoldExec;
- the two planners, which aren't the operator, move to planner.rs.
Imports.
- Functions are called through their modules: compute::cast, future::try_join_all, detect_folds::resolve, logical_expr::col.
- Types are used unqualified: Ordering, Any, HashSet, ScalarValue.
- A trait used by one function is imported inside it.
#[must_use] on every function whose result a caller must use, with clippy's must_use_candidate on for the workspace so new ones get it too.
Enums over bool. The rule's private fallback flag is the enum Fallback { HostPlan, Hash }.
Panics and references.
- Fold::dimension documents when it panics.
- Functions that only read a schema take &DFSchema, not &DFSchemaRef.
- EinFoldExec reads its inputs by pattern rather than with unwrap.
Names.
- The dense kernel's am, bm, av, bv and ax … by are spelled out.
- A domain's keys are CellKeys with named fields, rather than three zipped slices.
- Combinator chains move out of for headers.
- The detection test's sweep returns a Ran with named counts.
No macro where a function will do. keys.rs's zero_signs! is a generic function.

Three of the rustfmt options the guide recommends (imports_granularity = "Module", hex_literal_case = "Lower", format_macro_matchers) exist only on nightly rustfmt. The code follows them by hand, and they aren't configured.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

Summary by CodeRabbit

New Features
- Query planning can automatically detect eligible folds before execution, with host-plan fallback available when a fold cannot run through the optimized path.
Bug Fixes
- Improved handling of floating-point negative zero when matching signed-zero-equivalent keys.
Refactor
- Updated fold planning and execution organization while preserving existing plan behavior.
Tests
- Expanded coverage checks for folds that are joined back to their domain, including both optimized and host-plan execution paths.

coderabbitai · 2026-10-26T20:23:22Z

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: bb24050e-78e4-4e48-bf43-6d58e01234cc

Commits

Reviewing files that changed from the base of the PR and between 1faf319 and a8ad375.

Files selected for processing: 16

Cargo.toml
crates/einfold-datafusion/examples/matmul_gradient.rs
crates/einfold-datafusion/src/detect_folds.rs
crates/einfold-datafusion/src/ein_fold_exec.rs
crates/einfold-datafusion/src/fold_node.rs
crates/einfold-datafusion/src/join_back.rs
crates/einfold-datafusion/src/lib.rs
crates/einfold-datafusion/src/planner.rs
crates/einfold-datafusion/tests/detect_vs_host.rs
crates/einfold-datafusion/tests/operator.rs
crates/einfold-ir/src/fold.rs
crates/einfold-kernels/src/dense.rs
crates/einfold-kernels/src/hash.rs
crates/einfold-kernels/src/keys.rs
crates/einfold-kernels/src/relation.rs
crates/einfold-kernels/tests/kernels_vs_sql.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

The change adds must-use linting and annotations, separates DataFusion fold detection and planner code into dedicated modules, and updates kernel key handling. The summarized dense-fold and hash-kernel behavior remains unchanged.

Changes: Fold API annotations

Layer / File(s)	Summary
Must-use annotations `Cargo.toml`, `crates/einfold-datafusion/src/fold_node.rs`, `crates/einfold-datafusion/src/ein_fold_exec.rs`, `crates/einfold-ir/src/fold.rs`, `crates/einfold-kernels/src/relation.rs`	The workspace enables the Clippy `must_use_candidate` warning. Public methods and constructors gain `#[must_use]` annotations. `Fold::dimension` documentation states that an out-of-range ID causes a panic.

DataFusion fold planning

Layer / File(s)	Summary
Detection and planner organization `crates/einfold-datafusion/src/detect_folds.rs`, `crates/einfold-datafusion/src/planner.rs`, `crates/einfold-datafusion/src/lib.rs`	Fold detection now represents fallback choice with `Fallback`. `EinFoldPlanner` and `EinFoldQueryPlanner` are implemented in `planner.rs`, and the crate exports them from that module.
Execution and joined-back references `crates/einfold-datafusion/src/ein_fold_exec.rs`, `crates/einfold-datafusion/src/join_back.rs`, `crates/einfold-datafusion/src/fold_node.rs`	Planner definitions are removed from `ein_fold_exec.rs`; the operator remains there. Joined-back logic and execution references use the reorganized detection and fold-node modules.
DataFusion call sites and checks `crates/einfold-datafusion/examples/matmul_gradient.rs`, `crates/einfold-datafusion/tests/operator.rs`, `crates/einfold-datafusion/tests/detect_vs_host.rs`	Expression constructors use the `logical_expr` module. The detection sweep returns named counts and checks joined-back dense and host-plan cases.

Kernel key handling

Layer / File(s)	Summary
Dense and host-key implementation `crates/einfold-kernels/src/dense.rs`, `crates/einfold-kernels/src/hash.rs`, `crates/einfold-kernels/src/keys.rs`	Dense domain metadata is consolidated into `CellKey` values consumed by `locate`. Host-key construction uses a generic helper with float-width-specific negative-zero predicates.
Kernel validation and imports `crates/einfold-kernels/tests/kernels_vs_sql.rs`	The tests use a module-level `HashSet` import and local `fmt::Write` imports. Set and formatting behavior is unchanged.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Possibly related PRs

xqlsystems/einfold#48: Introduced fold detection and planner types that this change reorganizes into dedicated modules.
xqlsystems/einfold#46: Introduced fold nodes, execution, and planners that this change separates and updates.
xqlsystems/einfold#44: Introduced kernel hash and key code directly refactored in this change.

Merge Risk: Minimal · up to a8ad3

No actionable merge-blocking issue was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 75.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly summarizes the primary change: aligning Rust file names, imports, and names with Google's Rust style guide.
Description check		The description gives a detailed rationale and summary of the mechanical style changes. It does not include the template headings, a completed "Closes #" entry, or testing details, but it remains subs…
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: Description check

Explanation

The description gives a detailed rationale and summary of the mechanical style changes. It does not include the template headings, a completed "Closes #" entry, or testing details, but it remains substantially complete and relevant.

Full details: Docstring Coverage

Explanation

Docstring coverage is 75.76% which is insufficient. 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 33 functions across 15 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 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.}

alxmrs

LGTM

🪭 Claude (einfold) Stacked on #53. The maintainer asked that einfold's Rust follow [Google's Rust style guide](https://google.github.io/styleguide/rustguide.html). This is the first of two pull requests doing that. It holds the changes to the published crates' APIs; #55 has the mechanical rest. - **Named fields over tuples.** `Operand::keys` held `(DimensionId, ColumnIndex)` pairs, and `Output::domain_key` a `(ColumnIndex, KeyEquality)` pair, read as `.0` and `.1`. The hash kernel's `key(i).0 .0` showed why the guide prefers names. They are now `Key { dimension, column }` and `DomainKey { column, equality }`, with `new` constructors. - **No API in non-leaf modules.** `einfold-kernels`' root defined its whole API next to its submodules. The root now only declares modules and re-exports. The API moves to leaf modules: `options.rs`, `relation.rs`, `schema.rs` and `algorithm.rs`. - **Options.** `Options`, a bag of options with public fields, implements `Default`, so callers can write `..Options::default()`. Its `unique_domain: bool` is the enum `DomainRows { Any, UniqueKeys }`. `fold` returns a `Folded { algorithm, batches }` rather than a pair. - **`#[non_exhaustive]`** on `FloatKeys` and `Algorithm`, which will grow (block-sparse and fused algorithms, other hosts' rules). `KeyEquality` stays exhaustive: SQL has exactly two equalities. Left as they are, on purpose: - The IR's structs (`Dimension`, `Operand`, `Output`, `Domain`) stay exhaustive for now. They describe the fold's vocabulary, which the guide exempts, and einfold's other crates build them with literals. Worth revisiting before `einfold-ir` is published. - `Arc` stays where Arrow's and DataFusion's APIs require it. No behavior changes: the same property tests pass, unchanged except for the new names. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS   ## Summary by CodeRabbit * **New Features** * Fold operations can now use either dense or hash-based processing, with automatic fallback when dense processing is unsuitable. * Folding supports an option to require unique domain keys, and reports when that requirement cannot be met. * **Improvements** * Fold result schemas now validate operand and domain compatibility, including required columns and data types.  --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

fold_node.rs holds FoldNode, detect_folds.rs DetectFolds, and ein_fold_exec.rs EinFoldExec, as Google's Rust style guide asks. The two planners, which aren't the operator, move to planner.rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

- Call functions through their modules (compute::cast, future::try_join_all, detect_folds::resolve), and use types unqualified (Ordering, Any, HashSet, ScalarValue). - Import a trait inside the function that uses it, where only one does. - Mark functions whose result a caller must use #[must_use], with the must_use_candidate lint on for the workspace. - The rule's fallback is the Fallback enum rather than a bool. - Say when Fold::dimension panics, and take &DFSchema rather than &DFSchemaRef where a function only reads it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

… tuples - The dense kernel's a_matrix, b_matrix, a_factors and b_factors, and positions a_x to b_y, rather than am, bm, av, bv and ax to by. - A domain's keys are CellKeys, each with its column, extent and stride, rather than three slices zipped together. - Combinator chains move out of for-loop headers. - keys.rs's zero_signs is a generic function rather than a macro. - The detection test's sweep returns a Ran with named counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

…mns (#56) 🪭 Claude (einfold) Stacked on #55. The last of the speedups that bring M1's gate past 4× (#49, ddx's `matmul` gradient program at n = 50,000, end to end; ddx is an XQL Systems project for automatic differentiation of SQL queries). - **Positions are computed by column, in parallel.** Each batch's key columns are read, and its rows' positions computed, a column at a time in 32 bits. The compiler vectorizes that, and batches run in parallel. A sequential pass then places the rows. Placing `a`'s 800,000 rows took about 5 ms on one thread. - **Blocks of columns too.** The output was split into blocks of 256 rows. ddx's Ā step has an output of 16 rows and 50,000 columns, so it ran on one thread. Blocks now split columns as well. Each cell's terms are still added in `z` order, so the bits are what they were. - **A parallel uniqueness check.** A domain's batches are checked in parallel, with an atomic mark per cell: of two rows that reach one cell, whichever marks it second sees the mark. **M1's gate (#49) is passed: 4.0–4.1× over five runs** (369 ms with einfold off, 90 ms on), with the same gradients. More than half of what remains is ddx's own work outside the folds: its key check (32 ms) and an elementwise step (17 ms). The three folds take 37 ms between them. About 130 non-test lines. **Invariants tested.** `kernels_vs_sql.rs` gains a shape of 3 rows by 5,000 columns, so that column blocks run; its invariants are unchanged: - the same rows as DataFusion's SQL, within float summation's bound; - exactly SQL's NULLs, NaNs, infinities and signed zeros; - the same bits across arrival orders, batchings and thread counts.  🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS   ## Summary by CodeRabbit * **Performance** * Dense-fold processing is more efficient for batched workloads and outputs with many columns. * Duplicate-key checks for domain data now run in parallel. * **Reliability** * Processing preserves deterministic accumulation order for large outputs.  Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

alxmrs mentioned this pull request Oct 26, 2026

Follow Google's Rust style guide in the IR's and kernels' APIs #54

Merged

This was referenced Oct 26, 2026

Place rows by vectorized positions, and split few-row outputs by columns #56

Merged

Add M1's gate benchmark: ddx's matmul gradient with einfold on and off #49

Merged

alxmrs force-pushed the m1/style-api branch from abe9207 to c5197a9 Compare October 27, 2026 03:40

alxmrs force-pushed the m1/style branch from 40a6960 to 96d6c97 Compare October 27, 2026 03:40

alxmrs force-pushed the m1/style-api branch from c5197a9 to e7ff8cc Compare October 27, 2026 04:37

alxmrs force-pushed the m1/style branch 2 times, most recently from 6953a34 to 5800535 Compare October 27, 2026 19:22

alxmrs force-pushed the m1/style-api branch from e7ff8cc to 1c6529d Compare October 27, 2026 19:22

alxmrs force-pushed the m1/style branch from 5800535 to c5baabb Compare October 27, 2026 21:18

alxmrs force-pushed the m1/style-api branch from 1c6529d to 48362d6 Compare October 27, 2026 21:18

alxmrs force-pushed the m1/style branch from c5baabb to d271792 Compare October 27, 2026 23:28

alxmrs force-pushed the m1/style-api branch from 48362d6 to 7973468 Compare October 27, 2026 23:28

alxmrs force-pushed the m1/style branch from d271792 to 7bc53e4 Compare October 27, 2026 23:58

alxmrs force-pushed the m1/style-api branch 2 times, most recently from 4aa4942 to 6f035be Compare October 28, 2026 00:15

alxmrs force-pushed the m1/style branch 2 times, most recently from 3b2c923 to f49c9a4 Compare October 28, 2026 01:05

alxmrs force-pushed the m1/style-api branch 2 times, most recently from cf6c883 to 91e91d6 Compare October 28, 2026 01:11

alxmrs force-pushed the m1/style branch from f49c9a4 to 861a891 Compare October 28, 2026 01:11

alxmrs commented Oct 28, 2026

View reviewed changes

alxmrs force-pushed the m1/style-api branch from 91e91d6 to bd8f8af Compare October 28, 2026 23:57

Base automatically changed from m1/style-api to main October 29, 2026 00:13

alxmrs and others added 3 commits October 28, 2026 17:13

alxmrs force-pushed the m1/style branch from 861a891 to a8ad375 Compare October 29, 2026 00:13

alxmrs merged commit 93616c6 into main Oct 29, 2026
6 checks passed

alxmrs deleted the m1/style branch October 29, 2026 00:28

This was referenced Oct 29, 2026

Run EinFoldExec where there is no Tokio runtime #57

Merged

Replace the hash algorithm with discrimination, and default to it #68

Merged

Review every pull request with CodeRabbit, and say what a reviewer decides #109

Merged

Navigation Menu

Repository navigation

Follow Google's Rust style guide in file names, imports and names - #55

Follow Google's Rust style guide in file names, imports and names#55
alxmrs merged 3 commits into
mainfrom
m1/style

alxmrs commented Oct 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 26, 2026 •

edited

Loading

Uh oh!

alxmrs left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

alxmrs commented Oct 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alxmrs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

alxmrs commented Oct 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 26, 2026 •

edited

Loading