fix: skip arithmetic and concat operands that Spark skips under ANSI mode by tlfua · Pull Request #7456 · apache/datafusion-comet

tlfua · 2026-11-03T08:05:44Z

Which issue does this PR close?

Part of #7246. It fixes the arithmetic and concat rows of that issue. OR, AND and IN were fixed by #7326, and the later sort keys of a top-K are left for a separate PR, so the issue stays open.

Rationale for this change

With ANSI mode on, native Comet evaluates an operand over the whole batch even on rows where Spark never evaluates it, so a query that Spark answers fails in Comet with CAST_INVALID_INPUT:

+, - and * evaluate the right operand only where the left one is not NULL (BinaryExpression.nullSafeCodeGen).
/, DIV and % evaluate the divisor first, and the dividend only where the divisor is not NULL (DivModLike.doGenCode, the same in Spark 3.4 and 4.1).
concat evaluates each argument only while no earlier one is NULL (Concat.doGenCode, Spark's default generated code).

For example SELECT n + CAST(txt AS INT) FROM t returns NULL in Spark wherever n is NULL, whatever txt holds, but fails in Comet when such a row has a malformed txt.

What changes are included in this PR?

The fix reuses NullShortCircuit, which already evaluates later function arguments only where no earlier one is NULL (#6716).

NullShortCircuit::wrap_in_order evaluates the arguments in a given order, so that the divisor of a division comes first. wrap keeps the order of the children.
MathExpr gains null_short_circuit. The serde sets it when the operand that Spark may skip can be skipped (canSkipEvaluation), which is the right operand of +, - and * and the left one of /, DIV and %. The planner then wraps the arithmetic, including the checked ANSI functions, decimal arithmetic and the Decimal256 remainder (create_modulo_expr gains a null_short_circuit flag), keeping CheckedBinaryExpr outermost so errors still carry the query context. The rebalanced LEGACY add and multiply chains are unchanged.
CometConcat wraps the native concat with withNullShortCircuit when it has more than one argument.
A masked argument that is deterministic may first be evaluated over the whole batch, selecting the rows only if that fails, as LazyLogical does since fix: evaluate the right operand of AND, OR and IN only for rows the left side has not decided #7326. Without speculation, a * (b - c) under ANSI, as in TPC-H Q1, was up to 2x slower on batches with NULLs, because selecting the rows costs more than the arithmetic. Whether to speculate depends on the cost of the argument:
- A cheap argument is always speculated. is_cheap is a list of what has been measured: columns, literals, integer and floating point +, - and * (including the ANSI checked_add, checked_sub and checked_mul), and widening casts, over cheap children only. It can grow as more shapes are measured.
- Any other argument, such as a cast that parses a string, decimal arithmetic or a string function, is speculated only when at least half the rows need it. Otherwise the rows are selected directly, as before this change.
is_deterministic moves from private to pub(super), accepts CheckOverflow, and judges a nested NullShortCircuit by its inner expression alone, so decimal and nested operands speculate too, and the check stays linear in the size of the tree.
unwrap_checked in the planner also looks through NullShortCircuit, so the redundant CheckOverflow over a wide decimal add, subtract or multiply is still dropped.
The NullShortCircuit and withNullShortCircuit docs state that the function must return NULL wherever an earlier argument is NULL without checking the later ones, since with speculation it can see any value there.
create_binary_expr lost its last caller and is removed. Its tests call create_binary_expr_with_options.

An operand that Spark does evaluate still fails as before. A wrapped operand is NULL only on rows whose result is NULL anyway.

The same bug is on branch-1.2, which has NullShortCircuit but not #7326, which this builds on. The backport is tracked in #7542.

How are these changes tested?

spark/src/test/resources/sql-tests/expressions/math/arithmetic_ansi_skipped_operands.sql and spark/src/test/resources/sql-tests/expressions/string/concat_ansi_skipped_arguments.sql run every operator over a batch where the failing operand is malformed only on rows that Spark skips, over one where every row is skipped, and over mostly_null, where three rows in four are skipped, so a cheap argument (n * (big - 1)) is speculated, overflows on a skipped row and falls back to selecting rows, and a costly one (n + CAST(txt AS INT)) selects the rows directly. They cover tinyint, smallint, int, bigint, double, decimal and the Decimal256 paths, try_add and try_divide (including try_divide(CAST(txt AS INT), n) with a zero divisor, integer and decimal), and a zero divisor, where a NULL dividend gives NULL and a malformed one raises CAST_INVALID_INPUT before the zero check, as in Spark. expect_error_class(CAST_INVALID_INPUT) queries check that an operand Spark evaluates still fails, including the reverse order (CAST(txt AS INT) + n, n DIV CAST(txt AS INT)). Before the fix, the first query in each file fails with CAST_INVALID_INPUT.
Rust unit tests in null_short_circuit.rs, lazy_logical.rs and modulo_expr.rs: the divisor is evaluated first, it still fails where the dividend is NULL, order must be a permutation, the order survives with_new_children, speculative evaluation falls back to selected rows, a mostly-NULL batch selects the rows of a costly argument without speculating, a cheap one (checked_mul(a, checked_sub(b, c))) is speculated even with three rows in four skipped and still gives the right result when the skipped rows overflow, decimal and nested operands speculate, and is_deterministic checks 64 nested wrappers in linear time. test_check_overflow_skipped_over_short_circuited_wide_decimal in planner.rs checks that the redundant CheckOverflow is still dropped over a wrapped wide decimal add.
native/spark-expr/benches/null_short_circuit.rs compares the eager form with the wrapped one at 0, 10, 50, 70 and 90% NULL, for a + CAST(s AS BIGINT), a * (b - c), the decimal p * CheckOverflow(1.00 - d) in ANSI and LEGACY mode, a WideDecimalBinaryExpr multiply, a multiply by a rescaling decimal cast, and concat(q, upper(s)). Anyone can rerun it with cd native && cargo bench -p datafusion-comet-spark-expr --bench null_short_circuit.

µs per 8192-row batch, best of two rounds on a GitHub runner (run, from the measurement-only branch https://github.com/tlfua/datafusion-comet/tree/bench/7246, which adds a COMET_NSC_SPECULATE switch to force always or never speculating). "eager" is the unwrapped expression, as on main, which gives wrong errors.

Full table

shape	NULLs	eager	this PR	always speculate	never speculate
`a + CAST(s AS BIGINT)`	0%	96.1	96.3	96.3	96.3
	10%	94.1	97.1	97.0	114.5
	50%	92.2	80.9	94.1	80.8
	70%	89.5	48.2	92.3	48.2
	90%	89.5	18.4	91.2	18.3
`a * (b - c)` over longs	0%	14.4	14.8	14.8	14.8
	10%	15.4	15.8	15.8	34.4
	50%	12.4	12.7	12.7	20.0
	70%	10.6	11.0	11.0	13.3
	90%	9.3	9.7	9.7	7.4
decimal `p * (1.00 - d)`, ANSI	0%	41.7	42.1	42.1	42.4
	10%	46.3	46.7	46.7	57.6
	50%	33.5	35.5	33.9	35.4
	70%	26.8	22.5	27.1	22.6
	90%	20.8	11.1	21.2	11.4
decimal `p * (1.00 - d)`, LEGACY	0%	41.8	42.1	42.1	42.3
	10%	46.3	46.7	46.7	57.6
	50%	33.5	35.5	33.9	35.4
	70%	26.7	22.5	27.2	22.5
	90%	20.9	11.0	21.3	11.4
wide decimal `p * (d * d)`	0%	271.7	272.4	272.2	271.9
	10%	247.9	248.7	248.7	247.9
	50%	198.6	139.8	199.4	140.1
	70%	173.0	82.8	173.9	83.1
	90%	150.0	31.5	150.6	31.6
`p * CAST(d AS DECIMAL(12,4))`	0%	92.9	93.3	93.4	93.5
	10%	97.5	98.0	97.8	104.2
	50%	84.6	60.3	84.9	60.6
	70%	77.9	36.8	78.3	37.2
	90%	71.9	15.7	72.5	16.1
`concat(q, upper(s))`	0%	88.0	88.5	88.4	84.0
	10%	103.7	109.1	111.3	150.3
	50%	90.7	127.3	98.5	127.0
	70%	82.8	100.7	85.8	99.7
	90%	76.4	79.6	83.9	78.4

What the numbers show:

a * (b - c), the only shape on the cheap list, is within 5% of eager at every NULL ratio. Always speculating it is 1.2x to 2.2x faster than selecting rows at 10 to 70% NULL, and 2.3µs (1.3x) slower at 90%.
For the other shapes, always speculating is 1.2x to 5x slower than selecting rows at 70 to 90% NULL, so they keep the half-NULL threshold. That includes the wide decimal multiply and the rescaling cast, both 2.1x slower at 70% NULL.
concat(q, upper(s)) is the exception, and the only shape more than 6% slower than eager: 1.4x at 50% NULL and 1.2x at 70%, because selecting string rows is costly. Always speculating it would bring that to 1.09x and 1.04x and cost 5% at 90%, so infallible string functions are a candidate for the cheap list, but each needs measuring, so I left that for a follow-up.

Commands run locally (macOS, default Spark profile):

cargo test -p datafusion-comet-spark-expr --lib -- null_short_circuit lazy_logical case_when: 46 passed (on 9b6c7be).
cargo clippy --all-targets --workspace -- -D warnings, cargo fmt --all --check: passed (on 9b6c7be).
On dc1559c, before the latest commit: ./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite skipped" -Dtest=none, ... arithmetic and ... concat: passed; cargo test -p datafusion-comet --lib -- planner: 123 passed; ./mvnw spotless:check: passed.
cargo bench -p datafusion-comet-spark-expr --bench null_short_circuit: run on a GitHub runner, numbers above.

Fork CI (Comet CI (fork run), default Spark profile) passed on 9b6c7be, including the new SQL tests, the Spark 4.1 Comet suites and the TPC-H and TPC-DS result checks.

I did not run the Spark SQL suite or the other Spark profiles locally. This changes the serde and native expressions, so a maintainer may want to apply run-spark-4.1-tests, and run-benchmark-check for the benchmark changes.

…mode Spark's generated code evaluates the second operand of + - * only where the first is not NULL, the dividend of / div % only where the divisor is not NULL, and each concat argument only where every earlier one is not NULL. Comet evaluated every operand on every row, so an ANSI cast of a malformed string on a row that Spark returns NULL for failed the query. Wrap these expressions in NullShortCircuit, extended with an evaluation order so the divisor is evaluated first. A deterministic masked argument is first evaluated over the whole batch and only falls back to the selected rows if that fails, which keeps the common case as fast as eager evaluation. Part of apache#7246 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fbb6474-7dd6-4c0e-bb27-bd78a480ca36

tlfua · 2026-11-03T09:28:19Z

This changes the serde and native arithmetic and concat expressions, so could a maintainer please apply run-spark-4.1-tests to run the Spark SQL suite? The fork CI run passed: https://github.com/tlfua/datafusion-comet/actions/runs/41600696730

comphead

Thanks for picking this up. Reusing NullShortCircuit with an evaluation order keeps the change small. The order matches Spark 3.4.3, 3.5.8, 4.0.1 and 4.1.1: BinaryArithmetic.doGenCode uses nullSafeCodeGen for every numeric type including decimals, DivModLike evaluates the divisor first in both eval and doGenCode, and Concat.doGenCode stops at the first NULL.

Comparisons have the same shape. EqualTo and the ordering comparisons use defineCodeGen, so Spark evaluates the right side only where the left is not NULL. I expect WHERE n = CAST(txt AS INT) under ANSI to fail in Comet the way n + CAST(txt AS INT) did, but I have not run it. Does that belong under #7246, or in its own issue?

comphead · 2026-11-03T20:32:22Z

+        if nulls.is_some() && self.speculate.load(Ordering::Relaxed) {
+            if let Ok(value) = evaluate_projected(projection, projected, batch, None) {
+                return Ok(value);
+            }
+            self.speculate.store(false, Ordering::Relaxed);
+        }


LazyLogical only speculates when more than half of the rows need the right operand (pending_rows * 2 > num_rows in lazy_logical.rs). Here any NULL is enough, so when nearly every row is skipped the masked argument runs over the whole batch. That also applies to the existing users from #6716, such as array_contains, slice, make_date and GetMapValue, which on main evaluate an argument like CAST(s AS INT) over only the rows that need it. Could this use the same rule, for example nulls.null_count() * 2 < batch.num_rows(), and could the benchmark add a point at 90% NULL? I have not measured it.

The struct doc also still says the function sees NULL for each skipped argument. With speculation it sees the computed value there instead, so the function must also not fail on a row where an earlier argument is NULL, whatever the later ones hold. SparkArraySlice and SparkMakeDate check for NULL first, so I do not see a current bug, but could the doc here and the withNullShortCircuit Scaladoc say so, so that a new kernel does not validate first?

Thanks, measured it before picking a cutoff. The benchmark now also runs at 70% and 90% NULL, has a decimal shape, and I ran it on a GitHub runner always speculating and never speculating (run, from the measurement-only branch https://github.com/tlfua/datafusion-comet/tree/bench/7246, which is not part of this PR). The table is in the PR description.

Where selecting the rows starts to win depends on how much the argument costs. For a + CAST(s AS BIGINT) it is already faster at 50% NULL (65 vs 77 µs). For a * (b - c) it only wins at 90% (7.9 vs 8.6 µs), and at 50% it is 1.8x slower (20.2 vs 11.2 µs). So null_count * 2 < num_rows would cost the cheap arithmetic 1.8x at exactly half NULL.

I went with null_count * 2 <= num_rows, the same split as LazyLogical. For the three shapes it is at most 1.2x off the better choice up to half NULL and 1.4x above it, and at 90% NULL it is 4.5x faster than always speculating for the cast. A batch above the cutoff selects the rows without turning speculation off, so later batches still speculate. That also keeps the existing NullShortCircuit users from #6716 on the old path for mostly-NULL batches.

The doc now says the function must return NULL wherever an earlier argument is NULL without checking the later ones, both on NullShortCircuit and on withNullShortCircuit.

comphead · 2026-11-03T20:32:22Z

+        let expr = if options.null_short_circuit {
+            // Spark evaluates the divisor of a division first
+            let order: &[usize] = if op == DataFusionOperator::Divide {
+                &[1, 0]
+            } else {
+                &[0, 1]
+            };
+            NullShortCircuit::wrap_in_order(expr, order, &input_schema)?
+        } else {
+            expr
+        };


I think two things change for decimal operands once this wrapper is in the tree. I have not run it, this is from reading the code.

DecimalPrecision.promote puts every decimal +, -, *, / and % under CheckOverflow. The CheckOverflow arm skips the check for a WideDecimalBinaryExpr (perf: Optimize some decimal expressions #3619), but unwrap_checked only looks through CheckedBinaryExpr. For CheckedBinaryExpr(NullShortCircuit(WideDecimalBinaryExpr)), is_wide_decimal is now false and the extra CheckOverflow pass comes back. CAST(n AS DECIMAL(38, 0)) + CAST(txt AS DECIMAL(38, 0)) in the new fixture takes that path.

A decimal operand reaches NullShortCircuit as a CheckOverflow, which is neither in cannot_fail nor in is_deterministic. So price * (1 - discount) is wrapped even in LEGACY mode, where Spark returns NULL on overflow rather than failing, and speculation never applies to it. A batch with NULL prices then takes the selection path that the description measured at up to 2x for BIGINT.

Could unwrap_checked look through NullShortCircuit, and is_deterministic accept CheckOverflow? A decimal shape in the new benchmark would show whether that is enough.

You're right on both, thanks for reading it that closely.

unwrap_checked now also looks through NullShortCircuit, so the CheckOverflow over a wide decimal add, subtract or multiply is dropped again. test_check_overflow_skipped_over_short_circuited_wide_decimal in planner.rs plans CheckOverflow(a + CAST(s AS DECIMAL(38, 0))) under ANSI and checks that the result is the wrapped WideDecimalBinaryExpr with no CheckOverflow, and that it returns NULL for the row where a is NULL and s is malformed.

is_deterministic now accepts CheckOverflow, and a NullShortCircuit whose inner expression is deterministic, so decimal and nested operands speculate. decimal_and_nested_short_circuited_arguments_are_speculated covers both, and the benchmark has a decimal shape now.

I did not add decimal arithmetic or CheckOverflow to cannot_fail, because I can't show that the decimal kernels never return an error. So a LEGACY decimal operand is still wrapped, but with speculation it costs about the same as unwrapped (decimal rows in the table).

comphead · 2026-11-03T20:32:22Z

+statement
+INSERT INTO filled SELECT /*+ REPARTITION(1) */ * FROM VALUES (1, 1, 'bad'), (2, 2, '2') AS t(id, n, txt)
+
+-- Add, subtract and multiply evaluate the right operand only where the left one is not NULL


Could this file also cover the other paths the planner now wraps?

TINYINT and SMALLINT, which use the checked_* kernels on Int8 and Int16

DOUBLE for +, - and *, which stays a plain BinaryExpr

try_add and try_divide, which Spark plans as EvalMode.TRY arithmetic

a zero divisor under ANSI. Spark returns NULL for a NULL dividend over 0, and for CAST(txt AS INT) DIV n with txt = 'bad' and n = 0 it raises CAST_INVALID_INPUT rather than DIVIDE_BY_ZERO, because DivModLike evaluates the dividend before its zero check.

null_short_circuit_skips_the_dividend_where_the_divisor_is_null in modulo_expr.rs repeats the DECIMAL(38, 10) % DECIMAL(38, 0) query in this file, so it could go.

Added all of them:

TINYINT + and %, SMALLINT * and DIV

DOUBLE +, - and *

try_add and try_divide over skipped rows, and try_add with a malformed operand that Spark does evaluate, which still raises CAST_INVALID_INPUT

a zero divisor: DIV, % and / return NULL for a NULL dividend over 0, and CAST(txt AS INT) DIV n with txt = 'bad', n = 0 raises CAST_INVALID_INPUT

Comet matches Spark on all of them. I also removed the duplicate test in modulo_expr.rs.

comphead · 2026-11-03T20:32:22Z

+
+/// Like [`create_modulo_expr`], but with `null_short_circuit` evaluates the dividend only for the
+/// rows where the divisor, which Spark evaluates first, is not NULL. See [`NullShortCircuit`].
+pub fn create_modulo_expr_with_null_short_circuit(


After this change RemainderBuilder calls create_modulo_expr_with_null_short_circuit, so create_modulo_expr has no caller left outside its tests. The PR removes create_binary_expr for the same reason. Could create_modulo_expr take the null_short_circuit flag itself, so the crate keeps one public function instead of two?

Done. create_modulo_expr now takes null_short_circuit: bool, create_modulo_expr_with_null_short_circuit is gone, and the tests pass false.

- Speculate a masked argument only when at least half the rows need it, chosen from benchmarks at 0-90% NULL across three operand shapes. - Look through NullShortCircuit in unwrap_checked so the redundant CheckOverflow over wide decimal arithmetic is still dropped, and treat CheckOverflow and nested NullShortCircuit as deterministic so decimal and nested operands speculate. - Merge create_modulo_expr_with_null_short_circuit into create_modulo_expr. - Document that a wrapped function must return NULL where an earlier argument is NULL without checking the later ones. - Cover tinyint, smallint, double, try_add, try_divide and a zero divisor in the SQL tests, and drop the duplicate modulo unit test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fbb6474-7dd6-4c0e-bb27-bd78a480ca36

tlfua · 2026-11-03T22:20:41Z

Thanks for the review. I've pushed dc1559c for the comments above. The PR description has the updated benchmark table and test list.

On comparisons, you're right. SELECT id, n < CAST(txt AS INT) FROM cmp fails in Comet under ANSI where n is NULL and txt is malformed, while Spark returns NULL. WHERE n = CAST(txt AS INT) happened not to fail, since the inferred isnotnull(n) and the lazy AND skip the row first. I'd keep it out of this PR since it is already large, and opened #7468 for it.

tlfua · 2026-11-03T23:09:27Z

Fork CI is green on dc1559c (run). Since this round also changes native/spark-expr/benches/null_short_circuit.rs, could a maintainer please apply run-benchmark-check along with run-spark-4.1-tests? run-all-spark-profiles would also help, since the new SQL cases (try_divide, the zero divisor errors) run on every Spark version. Thanks!

coderfender

Thanks for the careful work here, and especially for the benchmark table. I went through Spark's BinaryExpression.nullSafeCodeGen, DivModLike and Concat.doGenCode in 3.4, 3.5, 4.0 and 4.1, and the order you evaluate operands in matches. Operands Spark does evaluate still fail, in both directions. I also like that the zero divisor case outside ANSI falls out of the existing nullIfWhenPrimitive and null_if_zero_primitive rewrites.

A few things I'd like to settle before merge.

native/spark-expr/src/conditional_funcs/null_short_circuit.rs:333. Compared with the release, which evaluates eagerly, your table shows a * (b - c) under ANSI going from 9.5 to 14.0 µs at 70% NULL, and it stays slower until about 85%. That shape is common under ANSI, and eager evaluation there only goes wrong if the subtraction actually fails on a skipped row. Could the select-rows path be kept for arguments where it pays off, such as ones containing a parsing Cast or a string function? Arguments built only from columns, literals and checked arithmetic would then always speculate first. If you'd rather keep a single threshold, could you say in the PR why the slowdown in that band is acceptable?
native/spark-expr/benches/null_short_circuit.rs:162. The bench already produces nulls_0, but the table leaves it out. Could you add it? No NULLs is the common case. TPC-H Q1's l_extendedprice * (1 - l_discount) now goes through the wrapper even in LEGACY mode, because its right side is a CheckOverflow and cannot_fail doesn't accept it. Could you also add a concat shape such as concat(s, upper(t))? cannot_fail rejects every ScalarFunctionExpr, so after this PR any concat whose later argument is a string function is wrapped in every eval mode.
spark/src/test/resources/sql-tests/expressions/math/arithmetic_ansi_skipped_operands.sql:170. Outside a failing eval mode, DivModLike also skips the dividend when the divisor is zero. Could you add SELECT id, try_divide(CAST(txt AS INT), n) FROM zero_bad, and a decimal variant, to pin that? Spark returns NULL there and the release raises CAST_INVALID_INPUT. A table where more than half the rows are NULL would also run the path that selects rows without speculating end to end. Right now skipped is exactly half NULL, so it always speculates first.
Backport. The description says branch-1.2 has the same bug. Could we either backport a version without speculation (1.2 has NullShortCircuit but not #7326), or file a tracking issue for it as backporting.md asks for a correctness fix?

This changes the serde, the planner and native expressions, so I'll make sure the Spark SQL and benchmark checks run before it goes in the queue.

rich7420 · 2026-11-05T19:15:16Z

+        // Its children are those of the function it wraps
+        || expr
+            .downcast_ref::<NullShortCircuit>()
+            .is_some_and(|n| is_deterministic(n.inner()));


Nested ANSI arithmetic doubles determinism work at each wrapper: n.inner() and the final all visit the same children. My 18-level helper probe counted 1,310,716 visits. Return the inner result directly.

Good catch, thanks. is_deterministic now handles a NullShortCircuit first and returns is_deterministic(inner), without visiting the children again, so the check is linear in the size of the tree. deeply_nested_short_circuits_are_checked_in_linear_time in lazy_logical.rs nests 64 wrappers, which would take 2^64 steps with the old code. Fixed in 9b6c7be.

Address round-3 review of apache#7456: - Speculate a masked argument over the whole batch regardless of the NULL fraction when it is cheap (columns, literals, integer and float +, -, *, ANSI checked_add/sub/mul, widening casts), so `a * (b - c)` is no slower than before the fix on mostly NULL batches. Costlier arguments keep the half threshold. - is_deterministic no longer visits the children of a NullShortCircuit twice, which was exponential in the nesting depth. - Benchmark LEGACY TPC-H Q1, wide decimal, rescaling decimal cast and concat(q, upper(s)) shapes. - SQL tests for try_divide with a zero divisor and for mostly NULL batches with cheap and costly operands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fbb6474-7dd6-4c0e-bb27-bd78a480ca36

tlfua · 2026-11-06T20:41:41Z

Thanks @coderfender, all four are addressed in 9b6c7be. The full table is in the updated description (bench run).

Done as you suggested. is_cheap in null_short_circuit.rs lists columns, literals, integer and floating point +, - and * (including checked_add, checked_sub and checked_mul) and widening casts, over cheap children only. Such an argument is always speculated first. Anything else, such as a parsing Cast, decimal arithmetic or a string function, keeps the half-NULL threshold. a * (b - c) under ANSI is now within 5% of eager at every NULL ratio (11.0 vs 10.6 µs at 70% NULL, where it was 14.0). I also benchmarked the borderline candidates. Always speculating a WideDecimalBinaryExpr multiply or a rescaling decimal cast is 2.1x slower than selecting rows at 70% NULL, and the decimal p * CheckOverflow(1.00 - d) 1.2x to 1.9x at 70 to 90%, so they stay off the list. concat(q, upper(s)) is the one shape where this falls short: with upper(s) kept off the list, as you suggested for string functions, it is 1.4x slower than eager at 50% NULL and 1.2x at 70%. Always speculating it would bring that to 1.09x and 1.04x, and cost 5% at 90%. So infallible string functions look worth adding to the list, but each needs measuring. I'd prefer a follow-up, but can add upper here if you'd rather.
The table now has nulls_0, and four new shapes: LEGACY p * CheckOverflow(1.00 - d) (your Q1 case), concat(q, upper(s)), a wide decimal multiply and a rescaling decimal cast. With no NULLs, the wrapper is within 3% of eager for every shape (a * (b - c) is 14.8 vs 14.4 µs, LEGACY Q1 42.1 vs 41.8).
Added try_divide(CAST(txt AS INT), n) FROM zero_bad and a decimal variant, both NULL as in Spark. The new mostly_null table has n NULL on three rows in four. n * (big - 1) is cheap, so it is speculated, overflows on a skipped row and falls back to selecting rows. n + CAST(txt AS INT), CAST(txt AS INT) DIV n and the two concat queries select the rows without speculating.
Filed Backport ANSI skipped-operand fix for arithmetic and concat to branch-1.2 #7542 to track the branch-1.2 backport.

github-actions Bot added bug Something isn't working area:expressions Expression evaluation labels Nov 3, 2026

tlfua marked this pull request as ready for review November 3, 2026 09:28

comphead reviewed Nov 3, 2026

View reviewed changes

tlfua mentioned this pull request Nov 3, 2026

ANSI: comparisons evaluate the right operand on rows where the left one is NULL #7468

Open

coderfender reviewed Nov 5, 2026

View reviewed changes

rich7420 reviewed Nov 6, 2026

View reviewed changes

tlfua mentioned this pull request Nov 6, 2026

Backport ANSI skipped-operand fix for arithmetic and concat to branch-1.2 #7542

Open

Conversation

tlfua commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

tlfua commented Nov 3, 2026

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlfua commented Nov 3, 2026

Uh oh!

tlfua commented Nov 3, 2026

Uh oh!

coderfender left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlfua commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlfua commented Nov 3, 2026 •

edited

Loading