[alxmrs]

🧭 Claude, continued by 🪭 Claude (einfold)

Stacked on #40 (DataFusion 55.2.0). First pull request of milestone M1, explicit folds for ddx (design §16). ddx is an XQL Systems project for automatic differentiation of SQL queries. M1 lets a plan producer like ddx emit a fold over a join directly, rather than SQL that einfold has to recognize. This pull request adds the node and a correct, general executor. The fast dense path comes next.

What a fold is here. A sum of products over a join of two operands, grouped by output dimensions:

SELECT <outputs>, SUM(a.value * b.value) FROM a JOIN b ON <shared dimensions> GROUP BY <outputs>

Each dimension's join compares with = or IS NOT DISTINCT FROM, as the producer says. A fold may carry a fill: a third relation whose rows the output has, each getting its group's sum, 0 where nothing reached the group, and NULL where the fill row's value is NULL. This is ddx's gradient step, which left-joins every input row to its gradient (dense_gradient in ddx-ad).

What changed.

• einfold-ir: Fold, Operand, Dimension, KeyEquality, Fill. Fold::try_new checks that the description is consistent, so holding a Fold is evidence that it is.
• einfold-datafusion:
  • FoldNode, a logical node, checks the inputs' columns and types, and derives the output schema.
  • EinFoldExec with the hash path: Gustavson's row-by-row sparse multiply, generalized to folds, so join rows never exist. It follows SQL's bag semantics (duplicates count), group existence (a group reached only by NULL products exists, and its sum is NULL), NULL keys per dimension (judged logically, so a dictionary's NULL value is NULL), and DataFusion's key equality: bitwise, except that -0.0 and 0.0 are one key, emitted as 0.0. DataFusion 55 made that change, and Pin DataFusion 55.2.0, in lockstep with ddx #40 pins it.
  • EinFoldPlanner and EinFoldQueryPlanner, which plan the node.
  • EinFoldExec reads its inputs, and every partition of each, concurrently, so an upstream repartition never buffers all its partitions but one.
• Determinism: each operand is visited in an order fixed by its rows' contents (keys, then factor), so a group's products are added in an order that doesn't depend on partitioning or batching (§8.6).

Invariants tested (tests/fold_vs_sql.rs, a property test). 400 random folds: one to four dimensions with BIGINT, VARCHAR, DOUBLE (including -0.0 beside 0.0, and NaN) or dictionary-encoded VARCHAR keys, the last with NULLs stored as a NULL dictionary value rather than a NULL index; each dimension in one or both operands; random outputs; a fill half the time. Tables have NULL keys, duplicate key tuples, missing rows, and factors that are NULL, NaN, ±inf, -0.0 or ill-conditioned (±1e16 beside small values), split across random partitions and batches. Each case checks:

1. the same rows as the SQL the fold stands for, run by DataFusion;
2. the same values within float summation's bound, 2·(n−1)·u·Σ|terms|, and exactly SQL's NULLs, NaNs, infinities and sign of zero;
3. the same rows bit for bit when the inputs arrive in other partitions and orders.

The oracle compares IS NOT DISTINCT FROM keys as the pair (key IS NULL, COALESCE(key, sentinel)) with =. DataFusion's hash join has one NULL-equality setting per join, and mixing both kinds in one join chain is bug #21.

Rebased onto DataFusion 55.2.0 (#40). Two commits on top:

• DataFusion 55's planner traits take a &dyn Session and a PhysicalPlanningContext, and ExecutionPlan lists its physical expressions (EinFoldExec has none).
• The property test failed on 55.2 at seed 1592651821: einfold split a GROUP BY on a DOUBLE into -0.0 and 0.0 groups, which DataFusion now merges. The hash path now folds -0.0 into 0.0 with DataFusion's own normalize_float_zero before encoding join and group keys, and emits the folded keys. A fill row keeps its own key, as a left join from the fill does, but finds its group by the folded one. Removing any of the three parts makes the property test fail.

Not in this pull request (stacked next):

• dense positional kernels in fixed, shape-chosen blocks (the speed);
• making EinFoldExec a full DataFusion operator: reserving memory from the memory pool, using every core, and reporting metrics. It collects its inputs whole and runs on one thread for now;
• the Fold Substrait relation, and ddx emitting it.

Size. About 615 lines of non-test code, not counting comments; over AGENTS.md's 400. If you'd rather, I can split the IR into its own pull request first.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

Summary by CodeRabbit

• New Features

  • Added fold-based query execution in DataFusion, including support for joined operands, optional fill data, and configurable output dimensions.
  • Added deterministic aggregation behavior across partitions and batches, with handling for nulls, duplicate rows, and floating-point keys.

• Compatibility

  • Updated the minimum supported Rust version to 1.94 and upgraded the DataFusion version to 55.2.0.

[coderabbitai]

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: d5f2ac05-a929-41b7-9f00-572591793d76

Commits

Reviewing files that changed from the base of the PR and between 653b785 and 53daada.

Files ignored due to path filters: 1

• Cargo.lock is excluded by !**/*.lock

Files selected for processing: 13

• .github/workflows/ci.yml
• CONTRIBUTING.md
• Cargo.toml
• crates/einfold-datafusion/Cargo.toml
• crates/einfold-datafusion/src/exec.rs
• crates/einfold-datafusion/src/hash.rs
• crates/einfold-datafusion/src/lib.rs
• crates/einfold-datafusion/src/node.rs
• crates/einfold-datafusion/tests/datafusion_behaviors.rs
• crates/einfold-datafusion/tests/fold_vs_sql.rs
• crates/einfold-ir/src/fold.rs
• crates/einfold-ir/src/lib.rs
• docs/lessons.md

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

Walkthrough

The PR adds a fold description to the IR crate and a DataFusion logical and physical execution path for folds. It adds hash-based aggregation, optional fill handling, tests against SQL and DataFusion behavior, and raises the minimum Rust version to 1.94.

Changes: Fold execution

Layer / File(s)	Summary
Fold description contract crates/einfold-ir/src/fold.rs, crates/einfold-ir/src/lib.rs	Adds public fold, operand, dimension, fill, and key-equality types. Fold::try_new validates descriptions and exposes fold metadata.
DataFusion logical fold node crates/einfold-datafusion/src/node.rs	Adds FoldNode, which validates its inputs and derives its output schema. It implements DataFusion’s logical-node interface and formats the fold for explain output.
Hash fold and physical planning crates/einfold-datafusion/src/hash.rs, crates/einfold-datafusion/src/exec.rs, crates/einfold-datafusion/src/lib.rs, crates/einfold-datafusion/Cargo.toml, Cargo.toml, .github/workflows/ci.yml, CONTRIBUTING.md	Adds hash-based fold execution and DataFusion planning for FoldNode. Updates the DataFusion pin to 55.2.0 and the minimum Rust version to 1.94. Adds async-trait and futures dependencies.
Behavior and SQL-oracle validation crates/einfold-datafusion/tests/*, docs/lessons.md	Adds tests for DataFusion float-key and NULL-join behavior, plus randomized comparisons of fold results against a SQL oracle. Updates the documented DataFusion float-key behavior.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant PlanProducer
  participant EinFoldQueryPlanner
  participant DefaultPhysicalPlanner
  participant EinFoldPlanner
  participant EinFoldExec
  participant FoldHash
  PlanProducer->>EinFoldQueryPlanner: Submit logical plan containing FoldNode
  EinFoldQueryPlanner->>DefaultPhysicalPlanner: Request physical plan
  DefaultPhysicalPlanner->>EinFoldPlanner: Plan FoldNode
  EinFoldPlanner->>EinFoldExec: Create operator with fold and inputs
  EinFoldExec->>EinFoldExec: Collect input partitions
  EinFoldExec->>FoldHash: Compute fold
  FoldHash-->>EinFoldExec: Return folded batch
  EinFoldExec-->>PlanProducer: Emit result slices

Loading

Possibly related issues

• Emit einfold's Fold relation for contraction steps (the interface for einfold's M1) ddx#208: This PR adds the Fold description, FoldNode, and EinFoldExec components named in the issue; it does not itself add ddx fold emission.
• DataFusion 55: dictionary-encoded floats keep -0.0 and 0.0 apart, unlike plain floats — draft for upstream #41: The PR tests DataFusion float-key behavior and normalizes signed zero in the fold hash path.

Possibly related PRs

• xqlsystems/einfold#40: Updates the same DataFusion dependency and Rust minimum version, and describes float and NULL behavior covered by this PR’s tests.

Merge Risk: Minimal · up to 53daa

This change adds a new fold planning and execution path in the DataFusion adapter, along with a Rust and DataFusion version bump. No concrete merge-blocking problem was found in the supplied evidence. The existing randomized comparison against SQL and the DataFusion behavior tests cover the main semantic risks.

Security architecture risk: Moderate · up to 53daa

The new execution path validates fold inputs and isolates each execution’s state, but buffers complete inputs and results without operator-level memory accounting. Large folds can also run lengthy computation without yielding. This creates a resource-exhaustion risk for applications processing untrusted data, although no externally exposed deployment was established.

Retained concerns

• Medium · security · inferred: The newly introduced fold operator materializes all inputs and the complete result without accounting for its own allocations through DataFusion’s memory pool. Its synchronous hash phase can perform every matching operand pair and retain every distinct output group without yielding. When callers permit attacker-influenced operand data or fold shapes, this can exhaust the host process or monopolize an executor worker despite output batch sizing; dropping the stream cannot cooperatively interrupt a hash phase already running. The planner is opt-in, and deployed attacker reachability remains unestablished.

Security review details

Security Blast Radius

• inferred — The resource-containment concern affects the process executing an activated fold. Other queries sharing that process or executor worker could be affected by exhaustion or starvation. Whether this reaches multiple tenants or an externally accessible service is unknown.

Security Findings and Attack Paths

• inferred — A caller able to influence operand cardinality or a producer’s fold shape can drive large allocations and pair enumeration. A valid fold with no shared dimensions matches every operand pair; distinct output keys can also retain a group for every pair. These allocations and computation occur before the first output batch, so output batch size is not an execution budget.

Trust Boundaries and Controls

• observed — Logical validation protects positional and type consistency, while explicit planner installation controls activation. Neither control bounds execution resources. The operand row-count guard protects u32 indexing and runs only after input collection; it is not a practical memory or work quota.

Resilience and Maintainability Implications

• observed — TaskContext is forwarded into child collection, so upstream execution retains that integration. After collection completes, however, the fold runs synchronously without a context, cancellation argument, or yield point. The cancellation limitation is established for this hash phase, not for DataFusion’s collection helper.

Hardening Proposals

• proposed — Account for input buffering, encoded keys, groups, and output through memory reservations with controlled failure or spill behavior. Bound pair work and group growth, and make CPU execution cancellation-cooperative rather than relying on output batch size or moving an uninterrupted loop to another thread.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 42.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 8 files. (5 skipped: …	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 identifies the main changes: adding the Fold node and the EinFoldExec hash path.
Description check		The description gives detailed motivation, implementation scope, testing coverage, DataFusion compatibility notes, and out-of-scope work. It does not include a completed "Closes #" issue link or the e…
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 detailed motivation, implementation scope, testing coverage, DataFusion compatibility notes, and out-of-scope work. It does not include a completed "Closes #" issue link or the exact template headings, but it provides the required information in equivalent sections.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 8 files. (5 skipped: 5 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]

🪭 Claude (einfold)

Closed in favor of the restacked M1, which follows design v17 (#42) and the review of this pull request: #43 (IR), #44 (kernels: hash), #45 (kernels: dense), #46 (DataFusion node and operator), #48 (detection), #49 (gate benchmark). Every fix made here carried over, including the DataFusion 55 float-key change and the UInt64 key fix, and so did its property test, ported to the new IR. This pull request and its branch remain as the record.