introduce StatisticsConverter::from_column_index - #9540
Conversation
|
Related to apache/datafusion#20871 |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @friendlymatthew -- sorry for the delayed review
| }) | ||
| } | ||
|
|
||
| /// Create a new `StatisticsConverter` from a Parquet leaf column index directly. |
There was a problem hiding this comment.
Thanks @friendlymatthew -- this makes sense to me.
However, I think we need at least some basic tests showing this working (aka create a Parquet file with a struct column and then read the statistics out of it using this API)
There was a problem hiding this comment.
Sounds good. You may also like this! apache/datafusion#21003 (comment)
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
a987c9e to
5d327c7
Compare
5d327c7 to
99d05b1
Compare
| ) -> Result<Self> { | ||
| if parquet_column_index >= parquet_schema.columns().len() { | ||
| return Err(arrow_err!(format!( | ||
| "Parquet column index {} out of bounds, max {}", |
There was a problem hiding this comment.
max reads as "highest allowed index", but it's actually the column count (count 1 → only index 0 is valid).
| @@ -2899,6 +2899,52 @@ mod test { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
should we extend the test to add:
- a non-Int32 leaf (e.g. Date64 or Decimal) to confirm
physical_typeis threaded correctly, and - an out-of-bounds index to cover the new error branch.
- A page-level check (data_page_mins/maxes) would be nice to have
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @friendlymatthew
Looks good 👍
|
Thank you for the review and help @kumarUjjawal 🙏 -- very much appreciated Thank you for the API and tests @friendlymatthew |
3 participants
Which issue does this PR close?
Rationale for this change
This PR adds a constructor StatisticsConverter::from_column_index that bypasses schema resolution and accepts a pre-resolved leaf column index directly
Datafusion's pruning predicate system needs stats for struct fields to support predicate pushdown on expressions like
WHERE my_struct.field > 67. Datafusion already resolves struct fields to Parquet leaf indices elsewhere, but has no way to pass that index to StatisticsConverter