[alxmrs]

🪭 Claude (einfold)

Nothing changes for users. Fixes #66, which never reached main, and adds a test that would catch it if it did.

#66 reported a planning failure: Cannot swap HashJoinExec inputs after dynamic filters have been constructed. DataFusion's physical optimizer (the rules, such as JoinSelection, that rewrite a physical plan) was optimizing a fold's fallback twice, where the fallback is the host's own plan, run where einfold's kernels decline. The fallback was optimized once when it was planned, and again as a child of EinFoldExec. That child was a commit (a49e236) in the stack before it was squash-merged. On main, EinFoldExec's children have only ever been its operands, and since #87 the fallback is a logical plan, planned only when it runs. So the optimizer can't reach it twice.

Decisions to review

• A test of the cause, not of Folds with a fallback can fail to plan: the physical optimizer runs over the fallback twice #66's query (the_optimizer_sees_a_fallback_once). A physical optimizer rule, Leaves, runs first in the session and keeps every leaf of every plan it's given. It fails if it's ever given one again. Recreating Folds with a fallback can fail to plan: the physical optimizer runs over the fallback twice #66 by making the fallback a child, on the commit before Plan a fold's fallback only when it runs #87, fails it with "the physical optimizer was given a plan it had optimized". It passes on main. The alternative was Folds with a fallback can fail to plan: the physical optimizer runs over the fallback twice #66's own query, but that passes even on a49e236: whether JoinSelection swaps depends on the tables' partitions and statistics, which the soak varied and the issue's SQL doesn't.
• The simulation's skip for Folds with a fallback can fail to plan: the physical optimizer runs over the fallback twice #66 goes (Known::FallbackOptimizedTwice in simulation.rs). It hid any error with Folds with a fallback can fail to plan: the physical optimizer runs over the fallback twice #66's message, so the simulation now reports one as a failure. A 5-minute soak without it ran 10,399 cases with no planning errors. Its 4 failures are Nightly fuzz: 102 property failure(s) in the simulation soaks #112's first four seeds, DataFusion bugs that Step the simulation around two known DataFusion bugs it was reporting #113 steps around.

Not in this pull request

#112's DataFusion bugs: #113 steps the simulation around them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M96xSez6PZwTmfaGh2eJgS

[coderabbitai]

Caution

Review failed

The pull request is closed.

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: xqlsystems/einfold
• Branch: m2/fallback-planned-once
• File: .coderabbit.yaml
• Commit: cb1cff9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3b1b9886-c64a-429f-9d34-28f9be6777b1

Commits

Reviewing files that changed from the base of the PR and between 908b5a5 and cb1cff9.

Files selected for processing: 2

• crates/einfold-datafusion/tests/detect_vs_host.rs
• crates/einfold-datafusion/tests/simulation.rs

Files with no reviewable changes: 1

• crates/einfold-datafusion/tests/simulation.rs

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 DataFusion tests add a physical optimizer rule that detects repeated plan leaves and exercise it on a host-fallback fold. The simulation removes the known-issue exemption for dynamic-filter join-swap errors, so host-success/einfold-error cases are reported as failures.

Changes: Fallback optimizer tests

Layer / File(s)	Summary
Repeated leaf detection in fallback planning crates/einfold-datafusion/tests/detect_vs_host.rs	A custom physical optimizer rule records plan leaves and asserts that it does not receive a leaf twice. A host-fallback fold test checks that the plan contains one fold and execution returns four rows.
Dynamic-filter error reporting crates/einfold-datafusion/tests/simulation.rs	The simulation removes the known-issue exemption for the dynamic-filter join-swap error. When the host succeeds and einfold errors, the simulation now reports the error.

Priority: Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Other

Merge Risk: Minimal · previous review cb1cf

The changes add regression coverage and expose simulation failures. No merge-blocking issue remains.

Pre-merge checks: 3 · 1 · 1

Failed checks: 1 warning · 1 inconclusive

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% 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.
Linked Issues check		Linked issue: #66 (open, direct). Requirement: a fold with a fallback must plan. The #66 reproduction query must plan at every target_partitions value from 1 to 4. The issue's suggested direction is t…	Provide evidence that, at the reviewed head, the planner no longer plans the fallback with create_physical_plan and no longer makes it a child of EinFoldExec. The inspected source paths are crates/einfold-datafusion/src/planner.rs, ein_fold…

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title is concise, specific, and accurately describes the added regression test for physical optimizer handling of a fold fallback.
Description check		The description explains the user impact, links the change to issue #66, documents the design decision, describes the regression test and simulation soak, and states what is out of scope. The testing …
Out of Scope Changes check		Both changed files relate directly to #66 and to the pull request objective. detect_vs_host.rs adds imports, a no-op physical optimizer rule named Leaves, and the test the_optimizer_sees_a_fallback_on…

Full details: Description check

Explanation

The description explains the user impact, links the change to issue #66, documents the design decision, describes the regression test and simulation soak, and states what is out of scope. The testing details appear without the template's explicit "## How it is tested" heading, but the required information is present.

Full details: Out of Scope Changes check

Explanation

Both changed files relate directly to #66 and to the pull request objective. detect_vs_host.rs adds imports, a no-op physical optimizer rule named Leaves, and the test the_optimizer_sees_a_fallback_once. The rule fails when it sees a plan leaf twice. This targets the double-optimization cause described in #66. simulation.rs removes the Known::FallbackOptimizedTwice entry from the Known enum and from KNOWN. It also removes the match arm that returned success for errors containing 'Cannot swap HashJoinExec inputs after dynamic filters'. That skip existed only for #66. Removing it lets this error surface as a failure. The diff contains no unrelated behavior changes. The PR description states that #112 and #113 work is not included here, and the diff contains no such changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% 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 3 functions across 1 files.

Resolution

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

Full details: Linked Issues check

Explanation

Linked issue: #66 (open, direct). Requirement: a fold with a fallback must plan. The #66 reproduction query must plan at every target_partitions value from 1 to 4. The issue's suggested direction is to optimize the fallback once, with the rest of the plan. This pull request changes only tests: detect_vs_host.rs and simulation.rs. No planner, EinFoldExec, or FoldNode source changes. The PR description says #66 is absent on main, because the fallback is a logical plan planned only when it runs (since #87). I could not verify this from the supplied evidence. The new test, the_optimizer_sees_a_fallback_once, does not run #66's SQL. It uses a different query over TEXT_TABLES and asserts one fold and four rows. Its pass or fail status on head is not shown in the evidence. The soak result (10,399 cases, no planning errors) is an author claim, not verified here. The issue also asks that the test pin both sides, using #66's reproduction. That reproduction is not covered by the new test. Requirement status is therefore unknown, not failed: the source-level claim is plausible but unconfirmed.

Resolution

Provide evidence that, at the reviewed head, the planner no longer plans the fallback with create_physical_plan and no longer makes it a child of EinFoldExec. The inspected source paths are crates/einfold-datafusion/src/planner.rs, ein_fold_exec.rs, and fold_node.rs. Also provide a passing run of the #66 reproduction SQL at target_partitions 1 through 4 on head. If the issue owner requires a test that uses #66's query, that test must also be added. Linked-issue scope is limited to #66 only.

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