Repository navigation
Attach RowFn validity during Boolean output construction - #10674
Closed
connortsui20 wants to merge 1 commit into
Closed
connortsui20 wants to merge 1 commit into
connortsui20 wants to merge 1 commit into
Conversation
connortsui20
added this pull request to stack #10677
October 21, 2026 15:10
Merging this PR will degrade performance by 11.12%3 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
connortsui20
force-pushed
the
ct/row-fn-cleanup/batch-inputs
branch
from
October 22, 2026 14:09
5541df9 to
abc8689
Compare
connortsui20
force-pushed
the
ct/row-fn-cleanup/output-validity
branch
from
October 22, 2026 14:09
14c6b46 to
263d116
Compare
connortsui20
removed this pull request from stack #10677
October 22, 2026 14:10
connortsui20
changed the base branch from
ct/row-fn-cleanup/batch-inputs
to
develop
October 22, 2026 14:10
robert3005
approved these changes
Oct 22, 2026
Let the packed Boolean collectors consume the batch validity when constructing their result. Keep custom output validation before masking and attach validity directly to other canonical outputs. Extracted from #9979. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
marked this pull request as ready for review
October 23, 2026 00:43
connortsui20
force-pushed
the
ct/row-fn-cleanup/output-validity
branch
from
October 23, 2026 00:43
263d116 to
cfbd513
Compare
connortsui20
added
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.998x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.024x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.990x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.010x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.003x ➖, 0↑ 0↓)
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.996x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.998x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.994x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.988x ➖, 0↑ 0↓)
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 (0.991x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.001x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.005x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.996x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.997x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.995x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.000x ➖, 0↑ 0↓)
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 (0.996x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.998x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.003x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.002x ➖, 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.951x ➖, 2↑ 1↓)
datafusion / parquet / ns (0.819x ➖, 7↑ 1↓)
duckdb / vortex-file-compressed / ns (0.939x ➖, 1↑ 1↓)
duckdb / parquet / ns (0.880x ➖, 3↑ 0↓)
|
Member
Author
|
Superseded by #10742, which explores direct canonical-output validity attachment in one private finalization branch instead of passing validity through the visitor and collector interfaces. The measured validity benefit is real, so this is a scope reduction rather than a claim that the optimization is ineffective. The replacement remains a draft pending fresh isolated measurements and review of the abstraction concern from #9694. |
connortsui20
added a commit
that referenced
this pull request
Oct 26, 2026
## Summary - Tracking Issue: #9128 - Supersedes #10674 - Related discussion: #9694 - Independent float32 candidate: #10741 Attaches input validity directly to canonical RowFn output using the existing helper. Preserves kernel-output validation, relabeling, outer nullability, and the non-canonical mask fallback. `values.mask(valid)` builds and optimizes a `ScalarFnArray` to dispatch through parent-reduce rules. The benchmark-driven bypass stays inside RowFn to avoid adding canonical-specific dispatch to the generic builtin. It can be removed if `.mask()` gains equivalent direct dispatch. This revisits the abstraction concern in #9694. **Draft pending fresh isolated benchmarks on this head.** Saved results include regressions and do not establish a general speedup. <details> <summary>Saved benchmark evidence and validation</summary> The isolated validity snapshot `b1961452ea3505d4e2588e34b14c20ea177ed175` was compared with frozen `develop` at `a1693389301e446eec41373cb274d98a303d3ca0`. Measurements used Rust 1.99.1 / LLVM 23.1.1, 16 CGUs, no LTO, CPU 2, and 21 balanced paired blocks on Graviton3 C7g and Sapphire Rapids C7i. This was native wall time, not CodSpeed simulation. For nullable u64 addition with 64 rows and a right-hand constant, median paired time ratios were: | Target | Candidate / baseline time | 95% paired bootstrap interval | | --- | ---: | ---: | | NEON | 0.798 | 0.796–0.802 | | AVX2 | 0.850 | 0.846–0.852 | | AVX-512 | 0.846 | 0.844–0.855 | The isolated snapshot also had flagged regressions outside that case: roughly 3.4% slower ARM u64 multiply and up to 4.2% slower AVX2 byte comparisons. No regressions crossed the 3% flag on AVX-512 in that run. These movements remain part of the review, even where unchanged code is sensitive to compiler context or binary layout. A separate combined packing/validity candidate at `8d8ef8151454bbc73e1cd0b236567542a1401cbe`, based on `d834a11faa79370b4d2e0bbcc73195f451a8a212`, retained the small-batch gain but regressed nullable u64 add by about 12% at 8,192 rows and 18% at 32,768 rows on AVX2. Profiling found a byte-identical retry hot loop with essentially unchanged instructions per operation and more cycles. The precise placement-sensitive mechanism remains unresolved. That combined result does not isolate this PR. The combined candidate passed 225 RowFn tests and 232 binary-function tests on that newer base, including output validation, allocator, and deferred-error coverage. Separate native probes checked null-row arithmetic errors and comparison edge cases. The changed file passed the pinned formatter. These are saved checks, not fresh validation of this individual PR. This PR is rebased onto `c750fbd80882f78efd6a2703bc0f3e17f1aca9f9`. The patch is unchanged by the rebase. Tests and benchmarks were not rerun after it. </details> --------- Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
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
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
Extracts output finalization from #9979. Packed Boolean collectors can construct the result with its input validity attached, avoiding a second array construction.
Targets
developand can merge independently of the borrowed-decoding and batch-inspection changes.Changes
The validity handoff stays private to the executor. Other canonical outputs receive validity directly, while custom kernel outputs retain validation before masking can hide an invalid result. Existing allocator cases are retained, and new cases cover attached Boolean validity and canonical batch output.
The Vortex test suite was not run locally. Focused native measurements of the combined migration were run separately during the compiler investigation.