andygrove commented on PR #6238: URL: https://github.com/apache/datafusion-comet/pull/6238#issuecomment-5969722626
Thanks @parthchandra. I filed #6562 for the NULL-entry gap, and it turned out to be reachable. `IF(c, col, NULL)` over a nested column goes through DataFusion's `CaseExpr` `nullif` path, which keeps the children under the new NULLs. It also has a struct version, a NaN in a NULL struct's child, which reaches the manifest on every profile rather than only 3.4/3.5. Struct children survive `take`, so that one needs its own fix rather than more of `compact`. I also corrected the "Not covered" note in the description: arrow 59's `take` drops a NULL list entry's elements at the level it gathers, so it doesn't keep those ranges as I'd said. I added the note about `offsets` to `reaches_past`, with a zero-row case in the unit test. The sliced-batch writer test now has a `list<float>` column, a `list<struct<double>>` column and NULL entries in every nested column. The detector and `compact` unit tests cover the float, list-of-struct and NULL shapes as well. With the fix reverted, all four nested fields count 5 NaNs where 1 is expected, and mutating out the float or struct detection fails the detector test. On the label, `run-iceberg-tests` is already on, but it runs Iceberg's own Spark suites. `CometIcebergWriteActionSuite` only runs on 3.4/3.5 under `run-all-spark-profiles`, so I ran the regression test locally on 3.5 against this head and it passes. -- 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]
