peterxcli commented on PR #5407: URL: https://github.com/apache/datafusion-comet/pull/5407#issuecomment-5381871102
Thanks for the detailed review. I pushed [`784c316cf`](https://github.com/apache/datafusion-comet/commit/784c316cf4ccc884fcf1542a4b0beee6eb7d8f8d) with focused coverage for the six runtime cases and the cross-version SQL assertion: * **Existence defaults:** I used the preserve-and-validate alternative rather than rejecting every Variant default. Spark's schema-held `VariantVal` is transported as a scan-only `[value, metadata]` constant, every value stays paired with its required-schema index, and the native schema adapter supplies it only when the Parquet field is absent. A present default that cannot serialize falls back safely. * **Unicode object ordering:** reconstructed objects are normalized to Spark's Java UTF-16 order. For partially shredded input, the residual value is temporarily put in Arrow UTF-8 order for upstream unshredding and converted to Spark order afterward. The regression contains 32 keys, including `U+E000` and `😀`. * **Dictionary metadata:** dictionary-encoded `metadata` is decoded before `VariantArray::try_new`. This has a Rust unit test and an end-to-end native `SELECT v` test using a generated Parquet fixture, since SQL cannot request nested Arrow dictionary encoding. * **Native writes:** the Variant guard inspects the actual data-producing child beneath `WriteFilesExec`, keeping Variant input on Spark's writer. A focused write-plan test verifies that `CometNativeWriteExec` is not selected. * **Python rewrite:** `EliminateRedundantTransitions` does not create `CometMapInBatchExec` when its input or output contains Variant. The Python operation remains on Spark's ordinary path, covered by a focused physical-rule test. * **Top-level NUL-containing FFI names:** only the exported C field name substitutes NUL with `U+FFFD`; datatype, nullability, and Field metadata are preserved. Rust and native-scan tests cover this boundary, and the source evidence is pinned to Spark `v4.1.3` and arrow-rs `58.4.0`. * **`PushVariantIntoScan`:** whole-value native SQL cases pin it to `false`, with a separate assertion that pushed `VariantStruct` remains an explicit fallback. Focused Rust tests, Spark 4.0/4.1 Variant SQL tests, the Spark 4.0 native-scan selection, writer and MapInBatch fallback tests, Spark 3.5 test compilation, formatting, Clippy, and `git diff --check` pass. I also refreshed the PR description so it documents the complete data path, compatibility handling, tests, and deliberate fallback boundaries in one place. -- 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]
