etseidl commented on code in PR #11348: URL: https://github.com/apache/arrow-rs/pull/11348#discussion_r4267042341
########## parquet/src/arrow/arrow_reader/statistics.rs: ########## @@ -1445,6 +1566,15 @@ where /// files, row groups, and data pages based on the statistics embedded in /// Parquet metadata. /// +/// # Statistics ordering +/// +/// This converter does not validate the file's column orders. Before using +/// INT96 timestamp bounds for pruning, the caller must check that the column's +/// footer entry is [`ColumnOrder::INT96_TIMESTAMP_ORDER`]. Legacy INT96 bounds +/// may use a different order and are not safe for timestamp pruning. Review Comment: I think my only problem with this is that it changes behavior and then leaves it up to users to figure things out. So if a downstream user has been relying on the fact that INT96 always returns null stats, and then all of a sudden they're not null, but also not guaranteed to be valid, they may use them without realizing the problem. I think this should either be explicitly opt-in by the user (so a flag that says return me INT96 stats, I know they're good), or we should change the converter and pass in the column_orders from the file metadata, and then only return stats if the order is INT96_TIMESTAMP_ORDER. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
