[SQL] Context-sensitive conditionals optimization by mihaibudiu · Pull Request #6335 · feldera/feldera

mihaibudiu · 2026-05-27T21:34:23Z

This implements a flow-sensitive dataflow analysis which tracks expressions that are constant within a conditional. For example, given: if x { ... x ... } else { ... x ... } in the then part of the if x can be substituted with true, and in the else part it can be substituted with false.

Checklist

Unit tests added/updated

Code generated before:

        let s4: Stream<RootCircuit, Tup2<bool, (Tup1<Timestamp>, )>> = s1.apply(
            move |p0: &Tup2<bool, Tup1<Timestamp>>, | -> 
            Tup2<bool, (Tup1<Timestamp>, )> {
                Tup2::new((*p0).0, (if (*p0).0 {
                    let p1: (&Tup1<Timestamp>, ) = (
                        &(if (*p0).0 {
                            Tup1::new(timestamp_trunc_week_Timestamp((*p0).1.0))
                        } else {
                            STATIC_0307dce7d0790fbf.clone()
                        }), 
                    );
                    (
                        (*p1.0), 
                    )
                } else {
                    STATIC_777571800a13a3a7.clone()
                }))
            }
        ).mark_distinct();

Code generated after:

        let s4: Stream<RootCircuit, Tup2<bool, (Tup1<Timestamp>, )>> = s1.apply(
            move |p0: &Tup2<bool, Tup1<Timestamp>>, | ->
            Tup2<bool, (Tup1<Timestamp>, )> {
                Tup2::new((*p0).0, (if (*p0).0 {
                    let p1: (&Tup1<Timestamp>, ) = (
                        &Tup1::new(timestamp_trunc_week_Timestamp((*p0).1.0)),
                    );
                    (
                        (*p1.0),
                    )
                } else {
                    STATIC_777571800a13a3a7.clone()
                }))
            }
        ).mark_distinct();

mythical-fred

Review

The optimization is well-targeted. The ContainsIf short-circuit avoids paying analysis cost on if-free expressions, and the EquivalenceContext-based comparison correctly handles alpha-renaming (testAliasedVariable proves it). The ResolveReferences.preorder(DBSPLetExpression) fix -- initializer in outer scope, body in inner scope -- is the right semantics that was missing, and dropping the let declaration's postorder cleanup into the inlined preorder is the right shape. Three IR-level unit tests cover the basic substitution, the equivalent-comparison case, and the alpha-renaming negative case.

Coverage gaps worth filling before merge

No SQL-level regression test. The PR description shows the timestamp_trunc_week codegen win beautifully, but there is no Regression3Tests case that compiles that SQL and pins the post-optimization shape. The IR-level unit tests are good but a SQL fixture is what catches future regressions in optimizer ordering -- e.g. the new CircuitRewriter(new SimplifyConditionals(...), false) placement before Simplify in CircuitOptimizer.createOptimizer, or interactions with the joins/limiters that run after it. Per the workspace rule on behavior changes without tests, this would normally block; soft-blocking here because the IR tests do exist and the description shows the wins, but please add at least one SQL-level fixture before merge.
No nullable-bool negative test. The optimization explicitly excludes nullable booleans (!expression.getType().mayBeNull in checkIfConstant). That is the right conservative choice today, but there should be a unit test that proves it: feed if (a: bool?) { a } and assert the inner a is not rewritten. Otherwise a future relaxation has no canary.

Inline notes

9 comments below covering: complexity bound, Scopes.clone() aliasing contract, enforce(subst != null) missing diagnostic, missing @Override, temp-file accumulation in QATests, and a coverage note on the MultiCrateTests.qaTests removal.

The big-picture design -- flow-sensitive dataflow over the IR using SymbolicInterpreter + EquivalenceContext for alpha-renamed equality -- is the right shape. Once the SQL-level test and the nullable-bool negative test land, this is good to go from my side.

mythical-fred

Re-APPROVE on 7796aa1. Two of my prior nits cleanly addressed: @Override added to postorder(DBSPIsNullExpression), and the Scopes.clone() lastClone/version memoization removed — eliminates the implicit aliasing contract and matches the explicit-copy semantics callers expect. The other three threads (constant-prop excludes nullable booleans; complexity bound on linear scan; enforce message in DBSPVariablePath) are non-blocking and can land as follow-ups. The replaceConnectors/deleteOnExit and MultiCrateTests doc nits also still apply but aren't blockers.

gz · 2026-06-24T18:14:58Z

+
+/** Simplifies expressions based on their context within a conditional;
+ * if (a) { ... a ... } simplifies the second occurrence of 'a' to 'true'. */
+public class SimplifyConditionals implements IRTransform {


You'd think calcite would have this optimization pass already

These are expressions that are synthesized by our compiler, usually from inlining functions.

This happens when we fuse multiple consecutive operators; this is something Calcite cannot do in general, it's set of operators is too weak to represent all we need.

gz · 2026-06-24T18:18:44Z

+                                .appendSupplier(expression::toString)
+                                .append(" = ")
+                                .append(value);
+                        Utilities.putNew(this.constants, expression, value);


you need an Utilities method to put something in a Map?

putNew checks that the value doesn't exist already

gz

I like it

mihaibudiu · 2026-06-24T18:24:53Z

good, I will let you rest a bit and send you more of these.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu · 2026-06-25T18:44:13Z

I will merge this after the current release is finished, to minimize possible disruptions. We have enough changes for now.

mythical-fred

Squash-rebase of previously approved work onto main — no new substantive changes. Still LGTM.

mihaibudiu enabled auto-merge May 27, 2026 22:54

mythical-fred reviewed May 28, 2026

View reviewed changes

mihaibudiu force-pushed the issue5507 branch from 31f120c to 7796aa1 Compare May 28, 2026 18:03

mythical-fred approved these changes May 29, 2026

View reviewed changes

mihaibudiu requested a review from gz June 24, 2026 16:20

mihaibudiu force-pushed the issue5507 branch from 7796aa1 to e5919c2 Compare June 24, 2026 16:21

gz reviewed Jun 24, 2026

View reviewed changes

gz approved these changes Jun 24, 2026

View reviewed changes

mihaibudiu added this pull request to the merge queue Jun 24, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 25, 2026

[SQL] Context-sensitive conditionals optimization

fa62d5d

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu force-pushed the issue5507 branch from e5919c2 to fa62d5d Compare June 25, 2026 18:31

mihaibudiu enabled auto-merge June 25, 2026 18:34

mihaibudiu disabled auto-merge June 25, 2026 18:43

mythical-fred approved these changes Jun 25, 2026

View reviewed changes

mihaibudiu added this pull request to the merge queue Jun 28, 2026

Merged via the queue into feldera:main with commit b734d02 Jun 28, 2026
1 check passed

mihaibudiu deleted the issue5507 branch June 28, 2026 07:13

Uh oh!

Conversation

mihaibudiu commented May 27, 2026

Checklist

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Review

Coverage gaps worth filling before merge

Inline notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

gz Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mihaibudiu Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mihaibudiu Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

gz Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

gz left a comment

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Jun 24, 2026

Uh oh!

Uh oh!

mihaibudiu commented Jun 25, 2026

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gz Jun 24, 2026 •

edited

Loading