tlfua commented on issue #7453:
URL:
https://github.com/apache/datafusion-comet/issues/7453#issuecomment-6760012733
Thanks @kazuyukitanimura, you're right. I traced every path that feeds a
native expression on `main` (8c145b742), and none of them can hand
`read_side_padding` a dictionary array today:
- **JVM input** (`ScanExec`, which covers JVM shuffle reads, broadcasts and
local table scans): `schema_from_data_types` unpacks every dictionary type, and
`build_record_batch` casts each column to that schema.
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/execution/operators/scan.rs#L195-L208
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/execution/operators/scan.rs#L309-L333
- **Native shuffle read** unpacks dictionaries before handing batches on.
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/execution/operators/shuffle_scan.rs#L221-L229
- **Native Parquet and Iceberg scans** adapt to the required Spark schema,
which unwraps dictionary columns even when the file's `ARROW:schema` declares
them. This is covered by `dictionary_columns_are_unwrapped_to_required_types`.
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/parquet/schema_adapter.rs#L3121
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/execution/operators/iceberg_scan.rs#L240
- **Target types** come from `to_arrow_datatype`, which maps `StringType` to
`Utf8` and never to `Dictionary`.
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/core/src/execution/serde.rs#L80
- **Other expressions:** no non-test code in `native/` builds a dictionary
array from a plain one. The only place that creates one is `read_side_padding`
itself, and only when its input is already a dictionary.
The arm was reachable when it was written. #1482 added the `Dictionary` arm,
and #2436 copied it into the three-argument case with `SPACE`. At that time
`ScanExec` still kept string dictionaries. #2635 then changed `ScanExec` to
unpack every dictionary type, which made the arm dead code.
https://github.com/apache/datafusion-comet/blob/8c145b742b1501087f5aef204bca4dd5d7f90a9e/native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs#L113-L137
So this is latent rather than a user-facing wrong result, and I don't think
it should be `priority:critical`. I'll leave the exact priority to the
maintainers. I'm happy to send a small PR that passes the pad string in both
`Dictionary` branches and adds a kernel unit test, so the arm stays correct if
a dictionary ever reaches it again. Would you prefer that, or should I close
this as not reachable?
--
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]
