fix(runend): clip min/max and is_constant to the array's logical window - #9960
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
`RunEndData::validate_parts` only requires the runs to *cover* `offset..offset + length`, so a run-end array may legally carry runs that lie entirely outside its logical window. `RunEndMinMaxKernel` and `RunEndIsConstantKernel` aggregated over every entry of `values()`, so they saw values no row of the array holds. Measured against the decoded array: for `try_new_offset_length(ends=[7, 10], values=[2, 3], offset=2, length=3)` -- logical rows `[2, 2, 2]` -- `statistics().compute_stat(Stat::Max)` returned 3 where the decoded array returns 2. That is the path that fills a file's zone map, and DataFusion and DuckDB read those statistics as column statistics. The error is always outward, min too low and max too high, so pruning stays sound and no rows go missing; what is wrong is the statistic itself. The is_constant kernel is one-directional in the same way, reporting a constant window as non-constant. The sibling aggregate in this crate already clips: `RunEndSumKernel` takes `array.offset()..array.offset() + batch.len()` and `sum/runs.rs` trims the boundary runs to it. Share that treatment through a `windowed_values` helper, which returns `values()` untouched when the window spans every run -- the common case, since the slice kernel trims both children. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
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
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.
RunEndData::validate_partsonly requires the runs to coveroffset..offset + length— the lastrun end must be
>=offset+length, and the first>=offset — so a run-end array may legally carryruns that lie entirely outside its logical window.
RunEndMinMaxKernel(
encodings/runend/src/compute/min_max.rs:40) andRunEndIsConstantKernel(
compute/is_constant.rs:37) aggregated over every entry ofvalues(), so they saw values no row ofthe array holds.
Measured against the decoded array, with the fix reverted:
ends=[7,10] values=[2,3] offset=2 length=3[2,2,2]Stat::Maxends=[4,9] values=[2,3] offset=4 length=2[3,3]Stat::Minends=[3,6,9] values=[1,5,9] offset=3 length=2[5,5]Stat::Minstatistics().compute_stat(Stat::Min)/(Stat::Max)is the path that fills a file's zone map, andboth engine integrations surface those as column statistics
(
vortex-datafusion/src/convert/stats.rs,vortex-duckdb/src/column_statistics.rs).Scope, stated plainly: the error is always outward — min too low, max too high — so zone-map pruning
stays sound and no rows go missing. What is wrong is the statistic. The
is_constantkernel isone-directional in the same way: it reports a constant window as non-constant, costing constant
folding, and can never report a non-constant window as constant. Going through the
min_max()helper returns the correct answer on these arrays, so this is not a wrong
MIN()/MAX()in a query.Vortex's own slice kernel trims both children (
kernel.rs:79), so it does not produce this shape;try_new_offset_lengthand a file whose run-end metadata declares a narrower window do.Fix
RunEndSumKernelalready clips —sum/kernel.rs:79takesarray.offset()..array.offset() + batch.len()andsum/runs.rstrims the boundary runs to it. Thisgives min/max and is_constant the same treatment through a
windowed_valueshelper, which returnsvalues()untouched when the window already spans every run.Tests
cargo test --release -p vortex-runend: 130 passed, 126 before.The first version of these tests was hollow: comparing
min_max(&array)withmin_max(&decoded)passed with the clipping removed, because that helper does not reach this kernel for these arrays.
They now assert on
compute_statand driveRunEndMinMaxKernel::aggregatedirectly, the waycompute/sum/tests.rsdoes. Withwindowed_valuesneutered, three of the four cases fail;window_spans_every_runpasses either way, which is the fast path.One case had to be corrected against the validator:
ends=[2,6,9]withoffset=3is rejected byvalidate_partswith "First run end 2 must be >= offset 3", so a leading run can only sit outsidethe window when it ends exactly at the offset.
AI assistance
Written with agentic AI assistance; the non-hollowness probe is what caught both the hollow first
test and the fact that the reachable path is the statistics one rather than the aggregate helper,
and the value claim above was narrowed to match.