Repository navigation
Preserve semantic errors in null checks - #10330
Open
connortsui20 wants to merge 5 commits into
Open
connortsui20 wants to merge 5 commits into
connortsui20 wants to merge 5 commits into
Conversation
Merging this PR will improve performance by 11.87%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | random_i128[0.01] |
10.1 µs | 9 µs | +11.87% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/preserve-null-check-errors (13ef05e) with develop (9324b04)
Footnotes
-
518 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
connortsui20
force-pushed
the
ct/preserve-null-check-errors
branch
from
October 7, 2026 10:47
f891323 to
5ed6bac
Compare
connortsui20
marked this pull request as ready for review
October 7, 2026 11:16
connortsui20
added
changelog/fix
github-actions
Bot
removed
the
action/bench-sql
Member
Author
|
big perf regression that I believe I can fix |
Fixes #10313. Null checks and validity reductions could drop a computation that fails. For example, `is_null` of a lazy cast of `[1, null]` to non-nullable `i64` returned `[false, false]`, but evaluating the cast fails. This change adds narrow guards only where a computation is dropped: - `ReduceNode::contains_fallible` reports whether a subtree has a scalar function that is not infallible. Array encodings and expression scope roots raise no errors of their own, so the walk passes through them. - `reduce_null` folds a non-nullable input to a constant only when the input cannot fail. - A reduced validity must keep the errors of its node. The default strict rule and `RowFn` now require an infallible function. `union_child_validities` keeps `is_not_null` of a fallible non-nullable child. `Cast` reduces only casts that add nullability. `FillNull` reduces only when its input cannot fail. An encoding with a fallible child has irreducible validity. - `IsNull` and `IsNotNull` execute a fallible input to columnar form before they read its validity. Infallible inputs keep every existing reduction, including the constant fold for non-nullable columns and the Kleene rewrites. Claude-Session: https://claude.ai/code/session_01FhBMXjkxtbQEuMqfAFvRmn Signed-off-by: Claude <noreply@anthropic.com>
Replaces four test functions with one table-driven test that compares each null check against the same check over the eagerly evaluated input, including the raised error. Each failing case targets one guard, and removing that guard fails only that case. The direct kernel path is dropped because the builtins path reaches the same kernel whenever the reduction declines. Claude-Session: https://claude.ai/code/session_01FhBMXjkxtbQEuMqfAFvRmn Signed-off-by: Claude <noreply@anthropic.com>
Replaces the term "drops" with what each rule does: it reads some inputs and never evaluates others. Adds small examples where they make a contract concrete, such as a cast of `[1, null]` to non-nullable `i64` and a dictionary over a failing cast. Each test fixture now states which rule it covers. Claude-Session: https://claude.ai/code/session_01FhBMXjkxtbQEuMqfAFvRmn Signed-off-by: Claude <noreply@anthropic.com>
Rewrites each comment so it reads on its own and states one rule with its reason. Names the `lit` rewrites in `reduce_null` instead of "the constant", and removes text that repeated the contract on `ReduceNodeValidity::Reduced`. Claude-Session: https://claude.ai/code/session_01FhBMXjkxtbQEuMqfAFvRmn Signed-off-by: Claude <noreply@anthropic.com>
Validity stays a metadata query that can skip the errors of a fallible input. `reduce_null` now applies the `lit` and `x.validity()` rewrites only when `contains_fallible` shows that x cannot fail, and the kernels still evaluate a fallible input. This restores symbolic validity for checked arithmetic and casts, which the `probe_scalar_fn_valid_*` benchmarks measure. Reverts the guards in the default strict rule, `union_child_validities`, `Cast`, `FillNull`, `RowFn`, and encoding validity. Drops the test cases that no longer cover a distinct guard. Claude-Session: https://claude.ai/code/session_01FhBMXjkxtbQEuMqfAFvRmn Signed-off-by: Claude <noreply@anthropic.com>
connortsui20
force-pushed
the
ct/preserve-null-check-errors
branch
from
October 7, 2026 13:31
b1a9ca9 to
13ef05e
Compare
connortsui20
commented
Oct 8, 2026
Comment on lines
+322
to
+337
| fn contains_fallible(&self) -> bool { | ||
| // A recursive search can overflow the call stack on deep expression trees. | ||
| let mut pending = vec![self.clone()]; | ||
| while let Some(node) = pending.pop() { | ||
| if node | ||
| .scalar_fn() | ||
| .is_some_and(|scalar_fn| !scalar_fn.signature().is_infallible()) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| pending.extend((0..node.child_count()).map(|idx| node.child(idx))); | ||
| } | ||
|
|
||
| false | ||
| } |
connortsui20
added
the
action/bench-sql
github-actions
Bot
removed
the
action/bench-sql
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Commits: PR datafusion / vortex-file-compressed / ns (0.995x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.994x ➖, 1↑ 1↓)
datafusion / parquet / ns (1.000x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.937x ➖, 4↑ 3↓)
duckdb / parquet / ns (1.005x ➖, 0↑ 1↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.994x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.001x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.004x ➖, 0↑ 1↓)
duckdb / parquet / ns (0.986x ➖, 1↑ 0↓)
No file size changes detected. |
myrrc
requested changes
Oct 8, 2026
myrrc
left a comment
myrrc
left a comment
Contributor
There was a problem hiding this comment.
I agree we should evaluate the input in the fallible case, yet I don't want to merge this change specifically because it's a massive regression. You now eagerly evaluate the subtree (which may be large) if it's fallible.
The good thing this shows we don't have the necessary benchmarks to see the regression.
Another good thing is that we can change is_fallible to take more information and reduce the cases where the expression actually is fallible.
I'll take on this task as agreed to make this not a regression
| /// IsNull(x) -> if !x.nullable lit(false) else not(x.validity()) | ||
| /// IsNotNull(x) -> if !x.nullable lit(true) or x.validity() | ||
| /// | ||
| /// These rewrites never evaluate x, so they require that x cannot fail |
Contributor
There was a problem hiding this comment.
This is the definition of symbolic reduction, can we remove this comment?
| /// is at most expensive as calculating x, but usually much cheaper. Although | ||
| /// in two cases you exchange 4 computations to 4 computations, the latter | ||
| /// four are cheaper. | ||
| /// four are cheaper. They still evaluate x and y, so they need no fallibility |
|
|
||
| /// Executes the input of a null check to columnar form if evaluating it can fail. | ||
| /// | ||
| /// [`ArrayRef::validity`] alone does not evaluate the array, so it misses the errors of a fallible |
Contributor
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.997x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.003x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.976x ➖, 5↑ 2↓)
duckdb / parquet / ns (0.980x ➖, 2↑ 1↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.001x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.001x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.016x ➖, 0↑ 4↓)
duckdb / parquet / ns (1.011x ➖, 4↑ 4↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.000x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.001x ➖, 0↑ 1↓)
duckdb / parquet / ns (0.996x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.925x ➖, 1↑ 2↓)
datafusion / parquet / ns (0.874x ➖, 3↑ 0↓)
duckdb / vortex-file-compressed / ns (0.863x ➖, 3↑ 1↓)
duckdb / parquet / ns (0.919x ➖, 1↑ 0↓)
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
is_nullof a lazy cast of[1, null]to non-nullablei64returned[false, false], but evaluating the cast errors.Changes
reduce_nullnow rewrites a null check without evaluating its input only whenReduceNode::contains_fallibleshows the input can't fail. Otherwise,IsNullandIsNotNullevaluate their input before reading its validity. Validity itself stays a cheap metadata query that can skip errors.Follow-up:
false AND xandmask(x, false)also skip evaluatingx.