peterxcli commented on PR #5234: URL: https://github.com/apache/datafusion-comet/pull/5234#issuecomment-5182600299
@andygrove Thanks for the detailed review, especially for verifying that the generic Parquet fallback fully covers the deleted branch. After tracing the Spark and Comet paths, I ended up separating dictionary sources from dictionary targets. ### Dictionary targets I confirmed that a real Spark plan cannot request a `Dictionary` cast target. Spark has no Dictionary SQL type, Comet’s protobuf type system cannot serialize one, and the normal scan/shuffle boundaries expose dictionary value types to downstream expressions. I also checked the historical CASE regression at runtime; its native coercion is now `Utf8` + `Utf8` → `Utf8`, not Dictionary. I therefore removed target packing instead of adding Boolean, non-`Int32` key, and non-string target coverage. [`cast_array` now rejects every Dictionary target explicitly](https://github.com/apache/datafusion-comet/blob/9242ea3ab3173ed1677c0c92a174fa63a9dd87ab/native/spark-expr/src/conversion_funcs/cast.rs#L223-L227), and the former target-success test is now a rejection test. This also removes the bare `arrow::compute::cast` packing call, so there is no longer a call that drops `native_cast_options`. With Dictionary targets unsupported, there is no remaining requested-key mismatch to track separately. ### Dictionary sources The source-Dictionary path is reachable during native Parquet schema adaptation. Spark ignores `ARROW:schema`, while arrow-rs honors it before adapting the physical array to Spark’s requested logical type. I rewrote that handler to use `as_any_dictionary` and `with_values`, so it is no longer hard-coded to `Int32` keys and no longer clones a newly owned dictionary. It Spark-casts the dictionary values first, then uses Arrow’s [`cast_with_options`](https://github.com/apache/datafusion-comet/blob/9242ea3ab3173ed1677c0c92a174fa63a9dd87ab/native/spark-expr/src/conversion_funcs/cast.rs#L243-L252) with `native_cast_options` to unpack the result. ### SQL coverage I added an [end-to-end native Parquet test](https://github.com/apache/datafusion-comet/blob/9242ea3ab3173ed1677c0c92a174fa63a9dd87ab/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala#L88-L141). It writes an unannotated Parquet `BINARY` column with `ARROW:schema` declaring `Dictionary(Int32, Binary)`, enables Spark’s `binaryAsString`, and includes invalid UTF-8. As a negative control, removing the source-Dictionary handler makes the test fail: Spark returns `f�o`, while Comet returns `null`. With the handler, the results match. I ran it with Spark 4.1 and Spark 3.5. The hand-written Parquet dictionary branch remains deleted, and the synthetic Parquet Dictionary-target test was removed because its target cannot originate from a Spark schema. -- 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]
