Skip to content

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

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

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>
@robert3005

Copy link
Copy Markdown
Contributor

This might happen but every time we perform operation on RunEnd array we trim the values. How do you manage to arrive at this where values has extraneous entries?

@jackylee-ch

Copy link
Copy Markdown
Contributor Author

Nothing in the workspace builds one. The slice kernel and the Arrow import trim both children, filter re-encodes at offset 0, and encode, BtrBlocks and the zone map build fresh arrays at offset 0; scalar-fn push-down, cast, fill_null, validity and take_from keep whatever shape they are given. The way in is RunEnd::try_new_offset_length, since validate_parts only requires the runs to cover offset..offset + length, and deserialization, which goes through the same check.

is_sorted reads values() wholesale as well, so if this is worth guarding, a check in validate_parts that rejects runs outside the window (length 0 excepted) would cover every kernel and make the trimming you describe an invariant, instead of clipping kernel by kernel. array.rs is also in #10080, so I would do that once it lands. If you would rather keep the constructor permissive, I will close this.

@jackylee-ch

Copy link
Copy Markdown
Contributor Author

Closing this. Walking the kernels one at a time isn't the right shape: is_sorted
(compute/is_sorted.rs:53) reads values() wholesale too, so clipping min/max and
is_constant leaves a third kernel with the same gap, and patching each in turn doesn't
scale.

The clean fix is one invariant — have validate_parts reject runs that fall entirely
outside offset..offset + length (the zero-length case excepted) — so every current and
future kernel can trust values() without clipping. That changes construction semantics
and touches array.rs, which #10080 is rewriting, so it belongs in its own change once
that lands rather than as a per-kernel patch here.

Nothing in the workspace builds an out-of-window array today: the slice kernel and the
Arrow import trim both children, filter rebuilds at offset 0, and encode / BtrBlocks / the
zone map start at 0. The only way in is the public try_new_offset_length or
deserialization, so there is no live wrong result to rush. Recorded for the follow-up.

@jackylee-ch jackylee-ch closed this Oct 3, 2026
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.

2 participants