ErikBPF commented on PR #5732: URL: https://github.com/apache/datafusion-comet/pull/5732#issuecomment-5706206463
Sorry for the delay. @comphead, yes, I am continuing with this; I have pushed the review follow-up in 5550f425fbe6c4d3d6fcc98cc811bbac6149f16a. - Took your suggested minimal guard simplification with `DataTypeSupport.isComplexType`. The comment now states the retained-filter invariant: containers cannot be partition columns, so exact partition selection cannot eliminate their null checks from the post-scan filter. Existing conservative AND/OR/NOT conversion remains. - Moved Rust field lookup inside the List/LargeList dispatch arms, preserving the intended non-list error. The existing field `Arc` is reused unless nullable struct elements require widening a required field. Added a regression for the dispatch error. - Moved the required-field projection regression to `CometIcebergNativeSuite` with a temporary warehouse and removed the incidental `WHERE`, so it now explicitly checks the null list as well. Added a null struct element to the existing `get_array_struct_fields.sql` fixture. - Kept the explicit Scala expected-row assertions and generated nested-shape sweep. The SQL harness compares Spark and Comet but does not supply literal expected-row assertions, so moving the full matrix there would lose the independent oracle for empty versus null containers and null elements. The generated sweep retains coverage of arrays of structs and structs of arrays requested earlier. Rust metadata checks also remain. - Added the Iceberg availability guard and datatype/predicate failure context, narrowed the generator comment, and cleaned up the condition and documentation placement. Validation on the tested source: 724 Rust expression unit tests and 7 integration tests passed; the native Iceberg and fuzz suites passed 114 tests on Spark 4.1, with one existing SPARK-55626 cancellation; the SQL fixture passed one test. Native build, Clippy with warnings denied, Rust formatting, Spotless, Scalastyle, and whitespace checks passed. These results cover Spark 4.1, not the full supported-version matrix; product CI is still pending. Thanks for the detailed review. -- 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]
