Skip to content

fix(runend): clip min/max and is_constant to the array's logical window - #9960

Open
jackylee-ch wants to merge 1 commit into
vortex-data:developfrom
jackylee-ch:fix/runend-minmax-window
Open

jackylee-ch wants to merge 1 commit into
vortex-data:developfrom
jackylee-ch:fix/runend-minmax-window

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

RunEndData::validate_parts only requires the runs to cover offset..offset + length — the last
run end must be >= offset+length, and the first >= offset — so a run-end array may legally carry
runs that lie entirely outside its logical window. RunEndMinMaxKernel
(encodings/runend/src/compute/min_max.rs:40) and RunEndIsConstantKernel
(compute/is_constant.rs:37) aggregated over every entry of values(), so they saw values no row of
the array holds.

Measured against the decoded array, with the fix reverted:

array logical rows statistic reported decoded
ends=[7,10] values=[2,3] offset=2 length=3 [2,2,2] Stat::Max 3 2
ends=[4,9] values=[2,3] offset=4 length=2 [3,3] Stat::Min 2 3
ends=[3,6,9] values=[1,5,9] offset=3 length=2 [5,5] Stat::Min 1 5

statistics().compute_stat(Stat::Min)/(Stat::Max) is the path that fills a file's zone map, and
both 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_constant kernel is
one-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_length and a file whose run-end metadata declares a narrower window do.

Fix

RunEndSumKernel already clips — sum/kernel.rs:79 takes
array.offset()..array.offset() + batch.len() and sum/runs.rs trims the boundary runs to it. This
gives min/max and is_constant the same treatment through a windowed_values helper, which returns
values() 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) with min_max(&decoded)
passed with the clipping removed, because that helper does not reach this kernel for these arrays.
They now assert on compute_stat and drive RunEndMinMaxKernel::aggregate directly, the way
compute/sum/tests.rs does. With windowed_values neutered, three of the four cases fail;
window_spans_every_run passes either way, which is the fast path.

One case had to be corrected against the validator: ends=[2,6,9] with offset=3 is rejected by
validate_parts with "First run end 2 must be >= offset 3", so a leading run can only sit outside
the 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.

`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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant