unikdahal commented on PR #5318: URL: https://github.com/apache/datafusion-comet/pull/5318#issuecomment-5500387835
Thanks @sunchao for the detailed review. I rebased this PR onto the latest `main` and addressed the remaining review findings. The current head includes the following fixes: * **Later predicates evaluating already-handled rows:** `run_group` now physically narrows the working batch after each matched instruction, so rows claimed by an earlier clause are never evaluated by later predicates. The ANSI divide-by-zero case from the review is covered by a regression test. * **Nested output nullability:** projected output now goes through `cast_and_stamp_schema`, matching the approach used by `ExpandExec`, so nested nullability/type widening is normalized before constructing the output batch. The reproduced nested-struct case is covered by a regression test. * **MERGE assignment subqueries:** `CometMergeRowsExec` now retains the source/target predicates and the three instruction groups as actual SparkPlan expression fields instead of hiding them under `originalPlan`. This lets Catalyst/Comet discover and prepare scalar subqueries correctly while also preserving MATCHED / NOT MATCHED / NOT MATCHED BY SOURCE boundaries for plan equality and canonicalization. Added an end-to-end scalar-subquery assignment test and semantic equality/canonicalization coverage. * **Cardinality hash-table memory accounting:** cardinality state now uses DataFusion's hash table implementation and reserves memory with the DataFusion memory pool before growing the table. Accounting includes hash-table capacity/allocation overhead rather than charging a fixed number of bytes per inserted ID, and reservation is rolled back if allocation fails. The exact 917,505-ID / 16 MiB reproduction from the review is covered, along with rehash-boundary and actual-allocation accounting tests. I also added/strengthened coverage for native `WriteDelta` execution and `Split` update-as-delete+reinsert semantics, required native engagement in the cardinality-error test, validated the row-ID type/ordinal up front, and kept `MERGE_CARDINALITY_VIOLATION` mapped to Spark's structured runtime error. Would appreciate another look when you have time. Thanks again for the precise reproductions, they helped tighten this substantially. -- 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]
