[emanuele-em]

Which issue does this PR close?

Part of #26302. This PR covers the boolean-literal comparison family only and does not close the full issue.

Rationale for this change

Keep the existing boolean predicate simplifications consistent when maintaining equality and inequality rules. The issue requests small, behavior-preserving changes, one rule family at a time.

The issue discussion also contains another contributor's plan for the same Eq/NotEq family: #26302 (comment). No matching open PR was found when preparing this PR. Coordination on this overlap is pending; this PR makes the completed refactor available for that discussion.

What changes are included in this PR?

• Share the boolean-literal Eq/NotEq rewrites through a private helper while keeping the flat guarded match structure and the existing type checks.
• Expand the existing comparison tests to cover both operand orders, nullable and non-nullable boolean columns, boolean NULLs, and nonboolean operands.

What is the testing strategy for this PR?

The existing Rust comparison tests disable canonicalization so both literal positions reach the simplifier directly. They also check the nonboolean type guards on raw expressions before type coercion. These are characterization tests for a refactor and passed before and after the production change.

Completed validation:

• cargo test --profile ci -p datafusion-optimizer: 941 unit tests, 29 integration tests, and 5 doctests passed; 1 doctest ignored.
• Ran the built SQL logic test binary with the simplify filter: all 4 matching files passed.
• cargo fmt --all and the CI Clippy configuration passed. TOML formatting, license headers, spelling, and Markdown formatting also passed.
• Compared the existing simplify_function_over_case simplifier benchmark against unchanged main at cf4869c. The three cases showed no statistically significant regression. This benchmark exercises the general simplifier and does not directly measure the boolean comparison family; no speedup is claimed.

Validation gaps:

• cargo clippy --all-targets --all-features -- -D warnings reported 3 mutable_key_type errors in unchanged push_down_filter.rs. A focused all-features Clippy run reproduced the same errors on unchanged main.
• The extended workspace run failed. Ordinary test binaries recorded 12,670 passes, 1 catalog lookup failure, and 8 ignored tests. The full SQL logic runner also failed with fixture parsing and path errors. Relocating the same binary exposed its compiled-in path normalization, so the full SQL suite cannot be reported as passing.
• The complete ./dev/rust_lint.sh suite did not finish; it was stopped at documentation generation after the checks listed above passed.
• The default cargo test -p datafusion and cargo test -p datafusion-cli commands have not been run separately. The extended workspace command excluded the CLI.

@alamb, could you help trigger CI for this first contribution? The validation gaps above remain open.

Are there any user-facing changes?

No intended changes to SQL results or public Rust APIs. This is an internal refactor of existing simplification rules.

[alamb]

Thansk for this PR @emanuele-em -- but to be honest I am not sure this is obviously more readable

Also I don't understand why we changed the tests

[alamb]

why is this changed?

[emanuele-em]

I added c2_non_null to check that the refactor preserves the same simplifications for non-nullable columns as for nullable c2. This adds coverage and it does not change the expected behavior. I can keep the original assertions and add that coverage separately to make the refactor easier to review if you prefer

[jayzhan211]

nit: I agree with @alamb here. Could you keep the original simplify(col("c2").eq(lit(true))) style assertions unchanged and add the new simplify_no_canonicalize loops alongside them? The originals cover the default path with canonicalization on, and leaving them untouched makes it easy to see the refactor didn't change any expected output. I ran the new loops against the pre-PR code and they pass there too, so the extra coverage is a good addition.

[emanuele-em]

Restored the original simplify(...) assertions unchanged in c4f2692, and kept the new simplify_no_canonicalize loops alongside them, so the diff in the tests is now purely additive. Thanks @alamb and @jayzhan211.

[jayzhan211]

nit: the (Some(_), _) arm is a catch-all, so if anyone ever calls this with another operator it silently becomes NOT other. Listing the four cases explicitly reads like the old rule table and might also help with the readability concern:

-    Ok(match (as_bool_lit(literal)?, op) {
-        (Some(true), Operator::Eq) | (Some(false), Operator::NotEq) => *other,
-        (Some(_), _) => Expr::Not(other),
-        (None, _) => lit_bool_null(),
+    Ok(match (op, as_bool_lit(literal)?) {
+        (Operator::Eq, Some(true)) | (Operator::NotEq, Some(false)) => *other,
+        (Operator::Eq, Some(false)) | (Operator::NotEq, Some(true)) => Expr::Not(other),
+        (Operator::Eq | Operator::NotEq, None) => lit_bool_null(),
+        _ => return internal_err!("unexpected operator {op} for boolean comparison"),
     })

[emanuele-em]

Done in c4f2692: the four cases are now listed explicitly and any other operator returns an internal error. Agreed it reads closer to the old rule table.

[jayzhan211]

nit: the helper doesn't check the operand types itself; the match guards at the call sites do. Maybe word it as "Callers must ensure both operands are boolean."

[emanuele-em]

Reworded in c4f2692, thanks.