fix: honor Kleene AND semantics for Between null bounds - #9404
Merged
Conversation
`Between` is not strict. Execution falls back to `lower <= arr AND arr <= upper` under Kleene `AND`, so a null bound still yields a definite `false` when the other comparison is false. `validity` conjoined all three children, declaring a row null whenever any bound was null, including rows that execution resolves to `false`. `Binary` returns `None` for `Operator::And` precisely because Kleene `AND` has no derivable validity expression, and `Between` desugars to that same `And`. Return `None` so the expression is evaluated and its mask extracted. Narrowing to the `arr` child would also be unsound, since a row can be legitimately null while `arr` is valid. Add a test that a declared validity agrees with the mask of the executed result, which covers this class of defect beyond `Between`. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`precondition` returned an all-null `ConstantArray` whenever either bound was a constant null. Under Kleene `AND` a null bound only makes a row null when the other comparison is not already false, so this nulled rows the surviving bound had already falsified. Because the branch keys off `as_constant()`, the result was also encoding-dependent: an all-null chunk stored as a `PrimitiveArray` produced `[false, null]` while the same chunk compressed to a `ConstantArray` produced `[null, null]`. Compression encodes all-null chunks as constants, so the same predicate over the same data could disagree from chunk to chunk. This also made `find_between` non-value-preserving. It rewrites conjoined comparisons with literal bounds into `Between`, so a null literal reached `precondition` through the standard optimizer. Under `not(...)` the rewrite changed a query's row count, since `NOT FALSE` is `TRUE` while `NOT UNKNOWN` is `UNKNOWN`. Short-circuit to all null only when both bounds are null, which is the one case where no comparison can falsify a row. With a single null bound, desugar into the two comparisons combined with Kleene `AND`, since the kernels all require non-null constant bounds. Reuse that desugaring for the existing fallback in `between_canonical`. `test_constants` asserted only that no row was `true`, which held under both the old and the correct result, so tighten it to the exact values. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
connortsui20
marked this pull request as draft
August 13, 2026 20:56
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | words_gather_scalar[65536] |
8.3 µs | 9.4 µs | -12.25% |
| ⚡ | Simulation | search_index_in_range_chunked |
5.8 ms | 5.2 ms | +12.05% |
| ⚡ | WallTime | words_gather_dispatch[1024] |
33 ns | 30 ns | +10% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/github-issue-9212-ia0abi (33cbc33) with develop (1a493b6)
Footnotes
-
89 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. ↩
Removes a contract stated three times. `precondition` documents that its result can be lazy, so the two call sites no longer repeat that in a comment. Replaces the copied null `ConstantArray` construction with a `null_i32s` test helper, and names why that encoding matters. Presents the `validity` test data as an annotated truth table rather than three columns, since the row is the unit the test reasons about. Renames `lower_null` to `lower_is_null` to read as the boolean it is, drops "may" and "should" from the docs, and adds the missing blank lines before returns. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`desugar` never said what it desugars into, and the term is off-register for an array compute crate. `Between` stands for two compares combined with Kleene `AND`, so the name now says that directly. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
connortsui20
force-pushed
the
claude/github-issue-9212-ia0abi
branch
from
August 13, 2026 21:31
2d3a6bc to
99d03c9
Compare
`precondition` can now return a lazy array, and the two execution paths force it because their callers expect a computed array. Both carry the same debt as the existing fallback TODO in `between_canonical`, so record it where the forcing happens. The reduce adaptor needs no marker, since a reduce rule can return a lazy array. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
Every other non-strict scalar fn in this family explains its `false`, and the trait default is also `false`, so a bare override recorded no intent. The comment states the reason and points at `validity`, matching `Binary`. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`between_canonical` still described the old `execute_boolean` fallback, so it now links [`as_two_compares`]. Trims the `is_strict` comment to point at `validity` instead of restating its reasoning, matching `Binary`, and settles on "compares" in both. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
connortsui20
added
changelog/fix
connortsui20
marked this pull request as ready for review
August 13, 2026 21:48
github-actions
Bot
removed
the
action/bench-sql
connortsui20
enabled auto-merge (squash)
August 13, 2026 21:48
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 1.000x ➖ datafusion / vortex-file-compressed / ns (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.004x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.997x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed / ns (1.010x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.000x ➖, 0↑ 0↓)
File Size Changes (9 files changed, -43.9% overall, 0↑ 9↓)
Totals:
|
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.998x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.022x ➖, 1↑ 1↓)
duckdb / parquet / ns (1.009x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -46.3% overall, 0↑ 2↓)
Totals:
|
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.997x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.999x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed / ns (1.015x ➖, 1↑ 6↓)
duckdb / parquet / ns (0.999x ➖, 4↑ 5↓)
File Size Changes (25 files changed, -43.5% overall, 0↑ 25↓)
Totals:
|
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.109x ❌, 0↑ 1↓)
datafusion / parquet / ns (1.013x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed / ns (0.986x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.989x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -42.8% overall, 50↑ 151↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.999x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.996x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.992x ➖, 1↑ 0↓)
File Size Changes (9 files changed, -44.0% overall, 0↑ 9↓)
Totals:
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.012x ➖, 1↑ 2↓)
datafusion / parquet / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.023x ➖, 0↑ 3↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 1↓)
File Size Changes (101 files changed, -39.2% overall, 0↑ 101↓)
Totals:
|
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed / ns (0.997x ➖, 1↑ 2↓)
duckdb / parquet / ns (1.000x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -32.3% overall, 0↑ 2↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.231x ➖, 0↑ 2↓)
datafusion / parquet / ns (1.204x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed / ns (0.981x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.813x ➖, 2↑ 1↓)
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.043x ➖, 1↑ 2↓)
datafusion / parquet / ns (1.160x ➖, 0↑ 5↓)
duckdb / vortex-file-compressed / ns (1.109x ➖, 0↑ 4↓)
duckdb / parquet / ns (1.076x ➖, 0↑ 0↓)
|
robert3005
reviewed
Aug 14, 2026
robert3005
approved these changes
Aug 14, 2026
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.
Rationale for this change
Betweenis not strict, so a null bound still gives a definitefalsewhen the other compare is false.validityandpreconditionboth treated it as strict and nulled those rows.preconditionkeys offas_constant(), so the result was also encoding-dependent: an all-null chunk stored as aPrimitiveArraygave[false, null]while the same chunk compressed to aConstantArraygave[null, null].validityandpreconditionbreak KleeneANDsemantics forBetweennull bounds #9212What changes are included in this PR?
validityreturnsNone, matchingBinaryforOperator::And.preconditionshort-circuits to all null only when both bounds are null, and otherwise builds the two compares under KleeneAND, since every kernel needs non-null constant bounds. That also replaces the existing fallback inbetween_canonical.For review, read the commits one at a time. The first two are the fix and the rest are names, comments, and tests.
validityis safe to land alone andpreconditionis not, so the order of the first two matters.The two execution paths force the now-lazy result because their callers expect a computed array, so both carry a marker beside the existing fallback TODO. The reduce adaptor passes it through unexecuted.
What APIs are changed? Are there any user-facing changes?
No public API change.
preconditionis crate-internal and gains anoptionsparameter. Filter results change for predicates with a null bound, which is the fix.StrictComparison::to_compare_operatorloses its last caller here. It stays because it is public andStrictComparisonis used across eight crates, and it goes away withBetweenitself.