Eagerly build single-conjunct filter evaluations - #9279
Draft
joseph-isaacs wants to merge 2 commits 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 6.34%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode_varbin[(1000, 2)] |
62.2 µs | 78.7 µs | -20.86% |
| ⚡ | Simulation | compress_fsst[(1000, 64, 8)] |
1.2 ms | 1 ms | +10.83% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/layoutreader-v1-scan-perf-efkqk3 (8cef37d) with develop (a99dd87)2
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. ↩
-
No successful run was found on
develop(75a3590) during the generation of this report, so a99dd87 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (10)
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 (1.031x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.010x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.001x ➖, 1↑ 2↓)
duckdb / parquet / ns (1.011x ➖, 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.000x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.010x ➖, 1↑ 2↓)
duckdb / vortex-file-compressed / ns (1.053x ➖, 0↑ 3↓)
duckdb / parquet / ns (1.000x ➖, 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 (1.016x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.003x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.081x ➖, 0↑ 7↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 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.019x ➖, 2↑ 5↓)
datafusion / parquet / ns (1.005x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.991x ➖, 6↑ 4↓)
duckdb / parquet / ns (0.996x ➖, 1↑ 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 (1.058x ➖, 1↑ 3↓)
duckdb / parquet / ns (0.997x ➖, 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.988x ➖, 2↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 2↑ 1↓)
duckdb / vortex-file-compressed / ns (1.032x ➖, 1↑ 22↓)
duckdb / parquet / ns (0.992x ➖, 6↑ 2↓)
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.919x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.986x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.042x ➖, 1↑ 1↓)
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.938x ➖, 1↑ 2↓)
datafusion / parquet / ns (1.022x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.819x ➖, 2↑ 0↓)
duckdb / parquet / ns (1.022x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.934x ➖ datafusion / vortex-file-compressed / ns (0.934x ➖, 2↑ 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 (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.999x ➖, 1↑ 1↓)
datafusion / parquet / ns (0.983x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 1↑ 2↓)
duckdb / parquet / ns (0.995x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -42.8% overall, 48↑ 153↓)
Totals:
|
## Rationale for this change Stacked on #9279 — that PR handles a single conjunct as a special case, this one generalises it to any number and deletes the special case. **Draft, and not mergeable as it stands: it is a large regression on prunable queries** (`lineitem_prune` +354%, `lineitem_and` +193%). Opening it because the mechanism works exactly as intended for the case it targets, and the regression isolates a specific, fixable blocker. Detail below. `LayoutReader::filter_evaluation` registers its segment reads when it is *called*, but only awaits its input mask when it is *polled*. The existing loop builds one evaluation, awaits it fully, then builds the next — so reads trickle out one conjunct at a time, per split, and nothing can be coalesced. The trait documentation already describes the intended alternative: > It is recommended to defer awaiting the input mask for as long as possible (ideally, after all I/O is complete). This allows other conjuncts the opportunity to refine the mask as much as possible before it is used. That only makes sense if several conjuncts' evaluations are built and in flight at once, which the caller never did. ## What changes are included in this PR? `chained_filter_mask` replaces both the single-conjunct helper from #9279 and the multi-conjunct loop. Net −31 lines. Each conjunct's output `MaskFuture` is fed straight into the next at construction time, so the reads for the whole chain are registered up front while each conjunct still receives the mask its predecessor refined — no extra compute, and the `EXPR_EVAL_THRESHOLD` low-density path still applies. The evaluation order is drained from `FilterExpr::next_conjunct` up front rather than re-queried between conjuncts. That is safe: `next_conjunct` (`scan/filter.rs:93-97`) reads a precomputed `ordering` vector that is only recomputed inside `report_selectivity`, from histograms accumulated *across* splits. Within a single split the order was already fixed. Ordering still adapts across splits. The `all_false` short circuit between filter evaluations is deliberately **not** carried over — see below. ### Results TPC-H `lineitem`, warm page cache, local NVMe. Row counts identical across all three variants on every shape. `pread64` counts: | Query | sf=1 base | sf=1 #9279 | sf=1 chain | sf=10 base | sf=10 #9279 | sf=10 chain | | --- | --- | --- | --- | --- | --- | --- | | `lineitem_filter_only` | 30 | 20 | **20** | 302 | 177 | **177** | | `lineitem` | 14 | 14 | 14 | 109 | 109 | 109 | | `lineitem_and` (2 conjuncts) | 13 | 13 | 14 | 14 | 14 | **109** | | `lineitem_prune` | 8 | 8 | 14 | 9 | 9 | **91** | | `lineitem_wide` | 114 | 113 | 117 | 1165 | 1172 | 1163 | Execution time at sf=10, median of 7 interleaved rounds, each round the median of 5 executions: | Query | base (ms) | chain (ms) | change | | --- | --- | --- | --- | | `lineitem_filter_only` | 140.3 | 151.8 | +8.2% | | `lineitem` | 91.5 | 109.7 | +20.0% | | `lineitem_and` | 27.7 | 81.1 | **+192.8%** | | `lineitem_prune` | 12.2 | 55.4 | **+354.1%** | Both regressions reproduce exactly across repeated runs (preads 14/14 vs 109/109 and 9/9 vs 91/91; timing distributions fully separated — `lineitem_prune` 12–14ms vs 45–57ms). This is deterministic, not noise. ### Why it regresses, and what would fix it The old loop checked `mask.all_false()` *before constructing* each conjunct's `filter_evaluation`. On a heavily-pruned query most splits never reached that line, so their filter reads were never registered at all — which is why the baseline is 9 preads on `lineitem_prune`. Chaining necessarily registers every conjunct's reads before pruning has run, because getting the I/O in flight early is the entire point. On prunable queries that is 10× wasted reads. Eager registration and pruning-driven skipping are therefore in direct tension, and the resolution has to live inside `filter_evaluation` rather than at the call site. Currently `flat::filter_evaluation` does: ```rust let mut array = array.clone().await?; // decodes unconditionally let mask = mask.await?; ``` It awaits the array *before* the mask, so an all-false input mask still pays for the read and the decode. Polling both concurrently and returning early when the mask resolves all-false would let cancellation propagate back up the chain and make eager registration close to free. That change is a genuine trade rather than a pure win — awaiting the array first is also what lets a conjunct's decode overlap its predecessor's compute, and short-circuiting gives that overlap up. Which effect dominates depends on selectivity, so it wants measuring on its own. These numbers say the waste dominates for prunable queries by a wide margin, so it is worth measuring next. ## What APIs are changed? Are there any user-facing changes? None. No public API changes; `chained_filter_mask` is a private helper. Results are unchanged — same masks, same arrays, same row counts. ### Checks run - `cargo nextest run -p vortex-layout -p vortex-file -p vortex-scan` — 350 passed - `cargo clippy -p vortex-layout --all-targets --all-features` — clean - `cargo +nightly fmt --all` — clean Not run: full workspace tests, Python bindings, docs — this touches one Rust file with no API or documentation surface. Co-authored-by: Claude <noreply@anthropic.com>
joseph-isaacs
added
the
action/bench-sql
github-actions
Bot
removed
the
action/bench-sql
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