Repository navigation
fix(writer): skip NaN min/max when aggregating Parquet column bounds - #3539
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Ensures Iceberg manifest bounds never record NaN by skipping NaN Parquet min/max statistics during min/max aggregation.
Changes:
- Skip updating min/max bounds when the extracted Datum value is NaN.
- Add a unit test covering all-NaN chunks and mixed NaN + real-value chunks for float/double columns.
| File | Description |
|---|---|
| crates/iceberg/src/writer/file_writer/parquet_writer.rs | Filters NaN bounds in MinMaxColAggregator::update and adds regression test verifying correct lower/upper bounds behavior. |
Review effort: Lite
| 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. |
There was a problem hiding this comment.
Good catch, reworded in 3ab570d: the comment now says a stored NaN bound would never be replaced by the non-NaN values of a later chunk.
| if !min_datum.is_nan() { | ||
| self.update_state_min(field_id, min_datum); | ||
| } |
There was a problem hiding this 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.
| if !max_datum.is_nan() { | ||
| self.update_state_max(field_id, max_datum); | ||
| } |
There was a problem hiding this comment.
Same as the min case above: leaving the guard inline.
Parquet 60 writes NaN min and max statistics for a column chunk whose values are all NaN, where earlier versions wrote none. MinMaxColAggregator copied them into the data file's lower and upper bounds, but the Iceberg spec does not allow NaN as a bound and iceberg-java records none. NaN is also unordered, so a stored NaN bound could not be replaced by a later chunk's value.
2b760bf to
fd65083
Compare
…ound Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM
Iceberg spec doesn't allow NaN as a bound:
NaNs are not permitted as lower or upper bounds.
From the spec
java does the same thing. the writer skips NaN when computing bounds, and if every value is NaN there's no bound (FloatFieldMetrics). when it reads bounds from the parquet footer, it drops them if either one is NaN (ParquetMetrics)
parquet 60 writes NaN min/max for a chunk where every value is NaN, so we need this before #3257 lands. it's a no-op on 59.
I added a commit on top (b82e58c): trimmed the comment and switched the test to -NaN so it covers the lower bound too (+NaN sorts last, so a later chunk overwrites it anyway)
|
thanks @andygrove |
|
Thanksk for the review @kevinjqliu! |
Which issue does this PR close?
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_maxinparquet/src/column/writer/encoder.rs).MinMaxColAggregator::updatecopies the Parquet statistics into the data file's lower and upper bounds without filtering them, so a data file with an all-NaNfloatordoublecolumn chunk gets NaN bounds in its manifest entry.The Iceberg spec says NaNs are not permitted as lower or upper bounds, 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 (#3257), the only effect is on statistics a writer reports as NaN.Are these changes tested?
Yes.
test_min_max_aggregator_skips_nan_boundsfeeds the aggregator an all-NaNfloatchunk followed by one with real values, and an all-NaNdoublechunk 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_datumand its max counterpart.