Eagerly build single-conjunct filter evaluations - #9279
Draft
joseph-isaacs wants to merge 1 commit into
Draft
Conversation
`split_exec` already builds the projection evaluation outside the returned future, so projection segment reads for every split are registered before any split task is polled and the IO system can coalesce them. The filter evaluation had no such treatment: it was built inside the `MaskFuture`, so a filter over a column that is not projected trickled its reads in one split at a time. The filter evaluation cannot be hoisted in general, because the conjunct order and the mask fed to each conjunct are chosen at runtime from selectivity statistics. When the filter has a single conjunct there is no ordering to choose, so the whole pruning-then-filter chain can be built at task-construction time instead. The pruned mask is awaited before the filter evaluation so that a split which pruning has eliminated entirely still drops (and therefore cancels) its filter reads, and the dynamic-expression re-pruning check is preserved. Measured on TPC-H lineitem with a filter on `l_linenumber` projecting only `l_extendedprice`, pread64 counts drop from 302 to 177 at sf=10 and from 30 to 20 at sf=1. Row counts are unchanged on every query shape measured. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6qV3R62EBNgkd2Leq5YqZ
joseph-isaacs
added
the
action/bench-sql
Claude
github-actions
Bot
removed
the
action/bench-sql
Merging this PR will degrade performance by 12.1%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.985x ➖, 1↑ 0↓)
datafusion / parquet / ns (1.004x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.015x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.006x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -46.3% overall, 0↑ 2↓)
Totals:
|
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.013x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.004x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed / ns (1.041x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.001x ➖, 0↑ 0↓)
File Size Changes (9 files changed, -43.9% overall, 0↑ 9↓)
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.988x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.008x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.031x ➖, 0↑ 2↓)
duckdb / parquet / ns (1.020x ➖, 0↑ 2↓)
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.011x ➖, 0↑ 3↓)
datafusion / parquet / ns (1.003x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (0.979x ➖, 7↑ 2↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 0↓)
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.942x ➖, 4↑ 1↓)
duckdb / parquet / ns (0.995x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -32.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.996x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.995x ➖, 2↑ 1↓)
duckdb / vortex-file-compressed / ns (0.978x ➖, 10↑ 2↓)
duckdb / parquet / ns (0.993x ➖, 3↑ 4↓)
File Size Changes (25 files changed, -43.5% overall, 0↑ 25↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.802x ➖, 3↑ 0↓)
datafusion / parquet / ns (0.956x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.960x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.782x ➖, 1↑ 0↓)
|
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 (0.969x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.028x ➖, 0↑ 3↓)
duckdb / vortex-file-compressed / ns (0.982x ➖, 1↑ 0↓)
duckdb / parquet / ns (0.998x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.963x ➖ datafusion / vortex-file-compressed / ns (0.963x ➖, 3↑ 0↓)
File Size Changes (1 files changed, +0.0% overall, 1↑ 0↓)
Totals:
|
joseph-isaacs
added
changelog/performance
Claude
github-actions
Bot
removed
the
action/bench-sql
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.945x ➖, 2↑ 0↓)
datafusion / parquet / ns (0.975x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.013x ➖, 1↑ 3↓)
duckdb / parquet / ns (1.003x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -42.8% overall, 47↑ 154↓)
Totals:
|
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
2 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
Draft — the I/O win is real but I could not turn it into a wall-clock win locally, and some shapes regress. Opening this so the repo's benchmark infrastructure can adjudicate on real hardware.
split_execalready builds the projection evaluation outside the returned future, so projection segment reads for every split are registered before any split task is polled and the IO system can coalesce them. The filter evaluation gets no such treatment — it is built inside theMaskFuture, so a filter over a column that is not projected trickles its reads in one split at a time.Measured on TPC-H
lineitemat sf=10, filteringl_linenumber > 5while projecting onlyl_extendedprice, that costs 302pread64calls versus 109 for the same filter when the filter column is projected. Splitting those reads by size: 150 of the 302 are under 1MB (p25 = 394KB) against a uniform ~2.26MB for the coalesced case. That population of small reads is the uncoalesced filter column.What changes are included in this PR?
One file,
vortex-layout/src/scan/tasks.rs.The filter evaluation cannot be hoisted in general: the conjunct order and the mask fed to each conjunct are chosen at runtime from selectivity statistics (
FilterExpr::next_conjunct,report_selectivity). But when the filter has a single conjunct there is no ordering to choose, so the whole pruning-then-filter chain can be built at task-construction time. That case is handled by a newsingle_conjunct_maskhelper; the multi-conjunct path is unchanged.Two behaviours are preserved deliberately:
MaskFutureisShared, so cloning it to do this is free.filter.dynamic_updates(0)version comparison) still runs at poll time, since it depends on runtime state.Results
pread64counts, TPC-Hlineitem, warm page cache. Row counts identical before/after on every shape at both scale factors.lineitem_filter_only(filter unprojected)lineitemlineitem_and(2 conjuncts, unchanged path)lineitem_prunelineitem_wideExecution time, median of 7 interleaved rounds (5 for
lineitem_wide), each round itself the median of 15 executions (5 at sf=10, 3 forlineitem_wide):lineitem_filter_onlylineitemlineitem_andlineitem_prunelineitem_wideSo: fewer, larger reads, but slower on this machine. Two effects explain it, and both argue that a local warm-cache benchmark is the wrong oracle for this change:
preadis nearly free here. The files are on local disk in page cache, so collapsing 302 reads into 177 saves syscalls but almost no latency. On object storage the request count is the dominant term.RepeatedScan::executebuilds every split task in a loop before spawning any of them, so anything hoisted intosplit_execleaves the worker threads and runs single-threaded ahead of execution.lineitem_pruneat sf=10 (+45.9%, before 13–15ms vs 17–20ms, cleanly separated) is the clearest case: that query prunes away most of the file, so constructing a filter evaluation per split is work discarded on splits that pruning then eliminates. The early exit cancels the reads but not the construction.I'd like the SQL benchmark suite to run before this is considered further; if it agrees with the local numbers, the honest conclusion is that this trade only pays off against remote storage and should be gated or dropped.
For the record, I first implemented the variant that hoists only the per-conjunct pruning evaluations. It changed pread counts by 0–1% and regressed
lineitem_filter_onlyby ~20% at sf=1 (n=15, barely-overlapping distributions), because zone maps and pruning results are already memoized per-reader —OnceLock<SharedZoneMap>andDashMap<BoundExpression, Option<SharedPruningResult>>inlayouts/zoned/pruning.rs— so registration order for those reads is irrelevant. That variant is not in this PR.What APIs are changed? Are there any user-facing changes?
None. No public API changes;
single_conjunct_maskis a private helper. Behaviour is intended to be identical — same result arrays, same masks.Checks run
cargo nextest run -p vortex-layout -p vortex-file— 326 passedcargo clippy -p vortex-layout --all-targets --all-features— cleancargo +nightly fmt --all— cleanNot run: the full workspace test suite, Python bindings, and docs checks — this change touches one Rust file with no API or documentation surface.
Generated by Claude Code