Fix logical error !raw_type->isNullable() in AND-compare chain optimization

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: error-handling, data-structures, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/111535

The problem

The ClickHouse logical expression optimizer could crash with a logical error (chassert(!raw_type->isNullable())) when optimizing certain AND-chains involving nullable types, especially when the chain's result type collapses to Nothing but some operands are still nullable. This was triggered by queries involving comparisons with NULL or nullable columns, leading to unhandled assertion failures in debug/sanitizer builds.

Why it's non-trivial

A naive fix might only check the AND's own result type for nullability, missing cases where nested or wrapped types (like LowCardinality(Nullable(T)), Dynamic, or Variant) yield nullable results even if their raw type does not. Also, comparisons with a constant NULL value must be handled carefully, as they have no meaningful value to prune or fold.

The approach

1. Guard against nullable comparison results

Previously, the optimizer only checked the raw operand type for nullability before pruning comparisons. This missed cases where the result type is nullable due to type wrappers or carriers. The fix adds a guard that checks the comparison node's result type instead:

+    /// A comparison with a nullable result is ambiguous under NULL and must not be pruned or folded;
+    /// keep it as-is. Test the comparison node's result type, not the raw operand type, so nested and
+    /// carrier-hidden nullability (e.g. `LowCardinality(Nullable)`, `Dynamic`, `Variant`) is caught.
+    if (isNullableOrLowCardinalityNullable(new_filter.original_node->getResultType()))
+    {
+        filter_map[expression].opaque_filters.push_back(std::move(new_filter));
+        return AddComparisonFilterResult::ADDED;
+    }

What it does:
This code ensures that if a comparison's result type is nullable (even if the operand's raw type isn't), the optimizer will not attempt to prune or fold it, but will keep it as an "opaque filter" (i.e., leave it untouched for later evaluation). This prevents the assertion from firing on complex or wrapped nullable types.

2. Remove unsafe assertion

The assertion that previously caused the crash is removed:

-    chassert(!raw_type->isNullable());

What it does:
This assertion is no longer valid since the new guard above handles all necessary cases. Its removal prevents the optimizer from aborting on valid but ambiguous nullable expressions.

3. Handle NULL-valued constants in comparisons

The optimizer now treats comparisons where the constant side is actually NULL as non-prunable, mirroring the logic used elsewhere for similar cases:

-            /// Both sides are non-constant — keep as-is.
+            /// Both sides are non-constant (or the constant is NULL-valued) — keep as-is.
             if (!constant)
             {
                 all_operands.emplace_back(argument_index, argument);

What it does:
This change ensures that if a comparison involves a constant NULL, it's not treated as a candidate for pruning or folding, but is instead kept as-is. This prevents logical errors when the optimizer tries to reason about values that don't exist.

4. Add comprehensive regression tests

A new stateless test file is added to cover all problematic cases, including:

• AND-chains with Nothing-collapsed results and nullable operands
• Chains with constant-NULL comparisons
• Chains involving LowCardinality(Nullable(T)), Dynamic, and Variant types
• Ensuring that redundant non-nullable comparisons are still optimized, while nullable ones are left untouched

Example (from the new test):

SELECT tuple((materialize(toNullable(NULL)) = 1) AND (assumeNotNull(materialize(toNullable(NULL))) = 2)) SETTINGS optimize_and_compare_chain = 1; -- { serverError ILLEGAL_COLUMN }

What it does:
This test ensures that the optimizer does not crash and that exceptions are properly handled, regardless of the chain optimization settings.

From the review thread

| a | Deterministic repro? | Yes. clickhouse-local -q "SELECT tuple((materialize(toNullable(NULL)) = 1) AND (assumeNotNull(materialize(toNullable(NULL))) = 2))" aborts every run (SIGABRT) at LogicalExpressionOptimizerPass.cpp:900. |

| c | Fix matches root cause? | Yes. The chassert documents the invariant "the pruned operand must be non-Nullable". The fix enforces exactly that at the point of use: a Nullable expression operand is kept as an opaque filter (no pruning), not asserted. |

Takeaways

• Always check the result type, not just the operand type, when handling type wrappers or carriers (e.g., LowCardinality(Nullable(T)), Dynamic, Variant).
  This ensures that all forms of nullability are correctly handled, even when nested or hidden.

• Do not attempt to optimize or fold expressions involving ambiguous or undefined values, such as comparisons with constant NULL.
  These should be preserved as-is to avoid logical errors and assertion failures.

• Comprehensive regression tests should cover both the error path (no crash/logical error) and the liveness of the optimization (other operands are still optimized when possible).
  This ensures both correctness and that the optimizer remains effective in mixed-type scenarios.