andygrove commented on code in PR #3539:
URL: https://github.com/apache/iceberg-rust/pull/3539#discussion_r4380709019
##########
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:
Good catch, reworded in 3ab570d01: the comment now says a stored NaN bound
would never be replaced by the non-NaN values of a later chunk.
##########
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:
Leaving this as is. Each guard is a single `!is_nan()` check, so a helper
would save one line per call site and add indirection. If more invalid-bound
rules appear, a shared predicate makes sense then.
##########
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:
Same as the min case above: leaving the guard inline.
--
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]
