andygrove opened a new pull request, #6658: URL: https://github.com/apache/datafusion-comet/pull/6658
## Which issue does this PR close? Closes #6562. Stacked on #6238, whose commits this branch includes until it merges. ## Rationale for this change The native Iceberg writer's `nan_value_counts` counted NaNs that Parquet never writes. iceberg-rust's NaN-count visitor reads each struct, list and map child whole, with only the child's own validity, so it counts a NaN under a NULL struct and the elements of a NULL list or map entry that still points at some. Arrow allows both layouts. `nullif` produces them, and Comet runs `IF(cond, col, NULL)` or `CASE WHEN cond THEN col END` over a nested column through it. The JVM carries the native NaN counts into the manifest, so they could exceed iceberg-java's: for the issue's reproduction, `s.x` reported 34 NaNs where iceberg-java reports 17. A NaN count that reaches a field's value count lets Iceberg's metrics evaluators treat the file as holding only NaNs. ## What changes are included in this PR? - `drop_unwritten_values` rebuilds every column that nests a float or double before a batch reaches the writer. A struct's fields become NULL wherever the struct is NULL, and a list or map keeps only the elements its non-NULL entries point at, at every depth. A column with nothing to drop, such as one from the Parquet reader or an Arrow builder, is handed back as-is, and a schema without a float inside a struct, list or map skips the pass. The Parquet data does not change, because the writer already skipped those values. - Gathering a list's or map's elements also compacts a batch that arrived sliced, which is what #6238's `RowSlicer::compact` did for #6146. That method and its helpers are removed, and #6238's integration test now runs through the new pass. - A pitfall entry in the contributor guide's Iceberg writes page, covering #6146 and #6562. ## How are these changes tested? - `nan_counts_skip_values_under_null_parents` writes seven nested shapes in which every float is NaN: a NULL struct, a NULL list entry and a NULL map entry that each still hold a value, a struct NULL under a NULL struct, a list under a NULL struct, NULL structs inside a list, and NULL lists inside a list. It runs each through the unpartitioned, fanout and clustered writers, over a whole batch and a slice of one, and compares the NaN count of each field with a hand-derived literal. It failed before the change with every NaN counted. - Two unit tests cover `drop_unwritten`: a column with nothing to drop is handed back unchanged, and a rebuilt column keeps every written value and NULL while losing the unwritten elements. Each fails under a mutation of the property it covers. - `CometIcebergWriteActionSuite` gains the issue's reproduction, `IF(id % 2 = 0, col, NULL)` over a double, a struct, a list and a map, and compares the native and iceberg-java `nan_value_counts`. Before the change it failed on Spark 4.1 / Iceberg 1.11 with `s.x` at 34 against 17. - `CometIcebergWriteActionSuite` passes in full on Spark 4.1 (84 tests), and the NaN tests pass on Spark 3.5, whose Iceberg 1.8.1 keeps list and map metrics, so the list element and map value counts are compared there too. Not covered: the separate `value_counts` and `null_value_counts` difference for a float under a NULL struct on Iceberg 1.10+, which the issue also describes. -- 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]
