andygrove opened a new pull request, #3539: URL: https://github.com/apache/iceberg-rust/pull/3539
## Which issue does this PR close? - N/A. I will file an issue first if the maintainers prefer one. ## What changes are included in this PR? Parquet 60 writes NaN min and max statistics for a column chunk whose values are all NaN, where Parquet 59 wrote none (`get_min_max` in `parquet/src/column/writer/encoder.rs`). `MinMaxColAggregator::update` copies the Parquet statistics into the data file's lower and upper bounds without filtering them, so a data file with an all-NaN `float` or `double` column chunk gets NaN bounds in its manifest entry. The Iceberg spec says [NaNs are not permitted as lower or upper bounds](https://iceberg.apache.org/spec/#manifests), and iceberg-java records no bound for such a column. NaN is also unordered in the aggregator's comparisons, so a stored NaN bound is never replaced by a later chunk's value. This PR skips a NaN minimum or maximum in `MinMaxColAggregator::update`. A column whose chunks are all NaN gets no bounds, and a column that mixes NaN chunks with others gets bounds from the non-NaN statistics. It needs no change to the Parquet dependency. Until the workspace moves to Parquet 60 (https://github.com/apache/iceberg-rust/pull/3257), the only effect is on statistics a writer reports as NaN. ## Are these changes tested? Yes. `test_min_max_aggregator_skips_nan_bounds` feeds the aggregator an all-NaN `float` chunk followed by one with real values, and an all-NaN `double` chunk on its own. It expects the real bounds for the first column and none for the second. It fails without the change. Downstream, DataFusion Comet's `CometIcebergWriteActionSuite` "NaN float/double manifest metrics match the JVM writer" compares the manifest metrics of the native writer against the JVM writer. On Parquet 60 the native writer produced `[1,0.0,NaN,2,0.25,NaN,3,NaN,NaN]` against the JVM writer's `[1,0.0,2.5,2,0.25,1.5,3,null,null]`. ## AI Disclosure The code, the test and this description were drafted with Claude Code (an AI assistant) and then reviewed by me. The place I'd most like a second look is whether skipping the bound belongs in the aggregator, as here, or in `get_parquet_stat_min_as_datum` and its max counterpart. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
