Copilot commented on code in PR #3539:
URL: https://github.com/apache/iceberg-rust/pull/3539#discussion_r4380436705
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -379,7 +379,12 @@ impl MinMaxColAggregator {
));
};
- self.update_state_min(field_id, min_datum);
+ // Iceberg does not allow NaN as a bound, and Parquet 60 writes
NaN min/max for a column
+ // chunk whose values are all NaN. Record no bound for such a
chunk, as iceberg-java
+ // does: NaN is unordered, so a NaN bound would also stay whatever
later chunks hold.
Review Comment:
The last sentence of the comment is misleading/unclear: if a NaN bound is
stored, later comparisons typically won’t replace it (because NaN is
unordered), rather than ‘stay whatever later chunks hold’. Consider rewording
to explicitly state that storing NaN would prevent later non-NaN stats from
updating/replacing the bound.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -390,7 +395,9 @@ impl MinMaxColAggregator {
));
};
- self.update_state_max(field_id, max_datum);
+ if !max_datum.is_nan() {
+ self.update_state_max(field_id, max_datum);
+ }
Review Comment:
The NaN-filtering logic is duplicated for min and max. Consider factoring
this into a small helper (e.g., `update_state_min_if_valid_bound` /
`update_state_max_if_valid_bound`, or a shared predicate like
`is_valid_bound(&Datum)`) to keep the update logic consistent and avoid
divergence if additional invalid-bound rules are added later.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -379,7 +379,12 @@ impl MinMaxColAggregator {
));
};
- self.update_state_min(field_id, min_datum);
+ // Iceberg does not allow NaN as a bound, and Parquet 60 writes
NaN min/max for a column
+ // chunk whose values are all NaN. Record no bound for such a
chunk, as iceberg-java
+ // does: NaN is unordered, so a NaN bound would also stay whatever
later chunks hold.
+ if !min_datum.is_nan() {
+ self.update_state_min(field_id, min_datum);
+ }
Review Comment:
The NaN-filtering logic is duplicated for min and max. Consider factoring
this into a small helper (e.g., `update_state_min_if_valid_bound` /
`update_state_max_if_valid_bound`, or a shared predicate like
`is_valid_bound(&Datum)`) to keep the update logic consistent and avoid
divergence if additional invalid-bound rules are added later.
--
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]
