Repository navigation
Conversation
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | sum_i64 |
194.2 µs | 224.8 µs | -13.62% |
| ❌ | Simulation | sum_v2_i64 |
195.1 µs | 225.8 µs | -13.59% |
| ⚡ | Simulation | copy_nullable[16384] |
424.2 µs | 313.2 µs | +35.46% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing polarsignals:statistices-nested-columns (cf42ed6) with develop (881b867)
Footnotes
-
534 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. ↩
|
I think for maximum compatibility this has to be a new top level field in https://github.com/vortex-data/vortex/pull/9775/changes#diff-9c5a36ce8f55480706984e8996f192cd3abbdd1b97afa6f702f646b3ce42e039L25. The problem you run into is that old readers already shipped code that make assumption about existing footer statistics. One issue is that they validate field count and if the count doesn't add up they will assume it's a corrupted file. The writer could optionally populate both of the segments and the new version will only ever read the new field. If you go that route you will see all the places that make assumptions about field stats that need fixing, i.e. datafusion, duckdb and jni |
|
We also want to use aggregate partials here? |
e24266f to
9c91f1a
Compare
|
@joseph-isaacs I've modified the summary stats to use aggregate partials. |
9c91f1a to
3b07ff7
Compare
9f6ce2f to
83660ba
Compare
e72a792 to
ee01b02
Compare
6a3a2a8 to
f8e7cf0
Compare
joseph-isaacs
added
the
action/bench-all
9c210b1 to
e0a709b
Compare
| Exact(Scalar), | ||
| /// An upper bound of the true maximum, either because the maximum was truncated or because | ||
| /// it was combined from a serialized partial, which doesn't record exactness. | ||
| Inexact(Scalar), |
There was a problem hiding this comment.
can we pull this out into a different pr
There was a problem hiding this comment.
Do you want to base this on a new exactness tracking PR, or do you want to drop this for now and always read back string min/max as approximate?
| fn partial_can_satisfy( | ||
| &self, | ||
| options: &Self::Options, | ||
| _partial: &Self::Partial, | ||
| requested: &AggregateFnRef, | ||
| ) -> AggregateFnSatisfaction { | ||
| self.can_satisfy(options, requested) |
myrrc
left a comment
myrrc
left a comment
There was a problem hiding this comment.
Duckdb changes look good to me, left some comments
Store file-level statistics via a post-order walk of the DType tree, so nested struct fields get whole-file pruning instead of only top-level columns. Each leaf gets a stats entry, and each nullable struct gets a trailing null-count entry. A new `is_nested` footer flag keeps old files readable via the legacy top-level-only layout. Signed-off-by: "Thor" <thor.hansen@dash0.com> Signed-off-by: Thor <thor.hansen@dash0.com>
Signed-off-by: Thor <thor.hansen@dash0.com>
Signed-off-by: Thor <thor.hansen@dash0.com>
| /// length against the number of top-level struct fields; omitting it means those readers find | ||
| /// no usable statistics in this file (though they can still read the data). Only use this if | ||
| /// you control every reader of the resulting files and don't need that compatibility. | ||
| pub fn exclude_legacy_statistics(mut self) -> Self { |
There was a problem hiding this comment.
We have a backcompat problem - when this is set the old readers will fail the read since they assert that the stats fields have the same length as the dtype. So we still need to make sure that holds.
There was a problem hiding this comment.
This defaults to true which shouldn't create a backwards compatibility issue though right? It's only for newer writers/readers that one would enable this option to not write the old stats. Or is the intent that we always will write the old stats to always support old readers?
There was a problem hiding this comment.
You don't need to write the old stats, you just need the number of entries in the old stats set to be equal to top level field count. Thus even if you enable this option the file would be readable by old readers just without any statistics - right now it just errors
5fd9add to
ccf038e
Compare
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
One thing that we have to do here is to ban any file stats for dtypes that have structural nullability so we cannot allow stats for nullable lists and structs all the way through since those stats would be wrong right now. We need to fix propagation of nulls through structural types before they're allowed
Signed-off-by: Thor <thor.hansen@dash0.com>
ccf038e to
cf42ed6
Compare
I didn't realize stats were broken for nullable nested types. We can implement this by having skipped fields point to empty AggregateSets. |
|
Right now we have https://github.com/vortex-data/vortex/blob/develop/vortex-layout/src/layouts/file_stats.rs#L468-L471. For non nullable Structs things are correct but at the moment vortex allows you to have parent nullability different from child which needs either changing or adding handling for |
|
I'm on PTO next week, will pick this back up after vacation |
This branch has not been deployed
4 participants
Summary
Being able to perform file level pruning on nested columns speeds up queries.
Changes
Adds post order stats to the file stats footer for nested columns. It adds a new
is_nested: bool = falseto the stats set flatbuffer for backward compatibility.Additionally it modifies the way the stats footer is accumulated to use the same AggRef functions that the zone maps use as well.
Only goofy thing it it also has to track truncatable stats separately because the aggregate partials don't have a way to express exactness.