Walkthrough

The pull request removes the filter composition pattern using AndFunctionFactory and wrapWithFilter helper method from SqlCodeGenerator, replacing it with direct FilteredRecordCursorFactory instantiations. It adds compile-time constant detection and post-join WHERE clause merging optimizations in SqlOptimiser, and simplifies the validateHorizonJoinGroupBy method signature. Test comments are updated to reflect the new filter composition behavior.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• #6824 — Modifies the same filter-wrapping logic in SqlCodeGenerator/FilteredRecordCursorFactory, creating a direct inverse relationship with this PR's filter composition changes.

Suggested labels

Bug

Suggested reviewers

• bluestreak01
• puzpuzpuz

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@CodeRabbit review

@bluestreak01, comments from Claude:

The core approach is sound. Merging at the AST level during optimization is architecturally cleaner than the runtime wrapWithFilter approach that combined compiled Function objects.
The optimizer already has concatFilters() for this exact pattern, and the method is used consistently elsewhere.

One concern with isCompileTimeConstant — mixed expressions:
If constWhereClause contains both a compile-time-constant part and a non-compile-time-constant part ANDed together (e.g., (1 > 10) AND (NOW() = NOW())), the top-level check returns false and merges the entire expression into postJoinWhereClause. This loses the EmptyTableRecordCursorFactory short-circuit that the code generator path provides.

With the old code, the compiled AndFunction could detect that one operand is constant false and report isConstant() == true, letting the code generator produce EmptyTableRecordCursorFactory. After this PR, the same expression is compiled as a row-level filter — it still filters all rows out, but it scans the data to do so.

This is a minor performance regression for an extremely unlikely query pattern. In practice, nobody writes WHERE 1 > 10 AND NOW() = NOW(). If the team cares, a possible fix would be to split the constWhereClause AND-tree into compile-time and non-compile-time subtrees, merging only the non-compile-time parts. But I wouldn't block on this.

isCompileTimeConstant — Narrow but Safe

  private static boolean isCompileTimeConstant(ExpressionNode node) {
      if (node == null) { return true; }
      return switch (node.type) {
          case ExpressionNode.CONSTANT -> true;
          case ExpressionNode.OPERATION -> isCompileTimeConstant(node.lhs) && isCompileTimeConstant(node.rhs);
          default -> false;
      };
  }

Observations:

1. Only checks lhs/rhs, not args. OPERATION nodes with paramCount > 2 would have children in the args list, which aren't checked. This is safe in practice since boolean operations are binary, but it's worth documenting or adding an args check for robustness.
2. Recursive without depth limit. Deeply nested constant expressions could stack-overflow. Not a realistic concern for constWhereClause which is typically shallow, but worth noting.
3. Conservative classification. Expressions like CAST(1 AS SHORT) > 10 contain a FUNCTION node (CAST) and would be classified as non-compile-time-constant. This is safe (they get merged) but misses an optimization. Acceptable tradeoff.
4. null returns true. Correct for the recursive case (unary operations have rhs == null), but a standalone call with null returns true. The caller guards with constWhere != null, so this isn't reachable, but the semantics are a bit surprising.

Test Coverage: Adequate but Could Be Stronger

Missing:

• No explicit test for compile-time constant preservation. A test verifying that WHERE 1 > 10 on a join query still produces EmptyTableRecordCursorFactory (via explain plan) would validate that the split between compile-time and non-compile-time is working correctly.
• No test for the edge case where constWhereClause mixes compile-time and non-compile-time parts (e.g., WHERE (1 > 10) AND (NOW() = NOW())). Even if the behaviour is acceptable, documenting it with a test prevents future surprises.

@glasstiger thanks for the thorough review. Addressing each point:

Mixed expressions ((1 > 10) AND (NOW() = NOW())) — Fixed in the second commit (3022bf6). mergeConstIntoPostJoinWhereClause now flattens the AND-tree with extractAndTerms() and classifies each conjunct independently. Compile-time terms stay in constWhereClause (preserving EmptyTableRecordCursorFactory folding), runtime terms move to postJoinWhereClause.

isCompileTimeConstant observations:

1. args list not checked — correct, boolean operations are always binary (paramCount=2, lhs/rhs only). No OPERATION node uses the args list.
2. Recursive depth — agreed, not a practical concern. constWhereClause is built from assignFilters conjuncts, which are individual filter terms AND-ed together — the tree depth equals the number of zero-table-reference terms in the WHERE clause.
3. CAST classified as non-compile-time — yes, this is conservative but safe. CAST is a FUNCTION node, so CAST(1 AS SHORT) > 10 gets merged into postJoinWhereClause and evaluated at runtime. Correct results, just misses the empty-table shortcut for an unrealistic query.
4. null returns true — only reachable in the recursive case (unary operations with null rhs). The top-level caller guards with constWhere == null early return.

Test coverage — both gaps are now covered:

• testNoArgMixedConstAndRuntimeExprInJoin (ExplainPlanTest) — asserts Empty table plan for WHERE 1 > 10 AND NOW() = NOW() on a join
• testJoinInnerPostJoinAndMixedConstFilter (JoinTest) — asserts empty result for mixed false + runtime
• testJoinInnerPostJoinAndMixedConstTrueFilter (JoinTest) — asserts correct result + plan for mixed true + runtime, verifying NOW()=NOW() appears in the post-join filter

[PR Coverage check]

😍 pass : 36 / 37 (97.30%)

file detail