andygrove commented on issue #4764: URL: https://github.com/apache/datafusion-comet/issues/4764#issuecomment-4905427616
## Investigation notes: Gap A (#4121) implementation constraint Digging into Gap A (native scan rejects invalid UTF-8), the "read the STRING column as Binary, then decode via `decode_utf8_spark_lossy`" direction runs into a constraint worth recording before anyone picks it up. ### The crux arrow-rs validates UTF-8 during Parquet decode, gated purely on whether the target Arrow field for the column is `Utf8`/`LargeUtf8` (the `parquet` crate's `arrow/buffer/offset_buffer.rs::try_push` raises `encountered non UTF-8 data`). If the target field is `Binary`, no validation runs. So the fix does require the reader to emit `Binary` for STRING columns, then decode `Binary` -> `Utf8` on the Comet side. The problem is choosing that read type. It is decided inside DataFusion's `ParquetOpener` via `options.with_schema(physical_file_schema)`, where `physical_file_schema` is computed by `apply_file_schema_type_coercions(logical, physical)`. That function only coerces **Binary -> Utf8** (plus view types), never **Utf8 -> Binary**. DataFusion 54's `ParquetSource` exposes no hook for us to override the read schema (no `with_schema_adapter`, and the opener does not consult a `SchemaAdapterFactory`). DataFusion has a `binary_as_string` option but no `string_as_binary`. So there is no clean Comet-only insertion point in stock DataFusion 54 to force STRING columns to be read as `Binary`. This applies to both the `native_datafusion` scan (`parquet_exec.rs`) and `native_iceberg_compat` (`iceberg_scan.rs`), which share the expr adapter. `native_comet` (the JVM reader) is unaffected. ### The Comet-side decode half is straightforward Independent of how we source the read-as-Binary capability: - Expose `decode_utf8_spark_lossy` (currently `pub(crate)` in `native/shuffle/src/spark_unsafe/unsafe_object.rs`), ideally by relocating it to `datafusion-comet-spark-expr` so both scan and shuffle depend on it and the cast path can reuse it later. Note `cast_binary_formatter` (`spark-expr/.../cast.rs`) still uses `from_utf8_unchecked` today, so any Binary -> Utf8 adapter cast must not reuse it. - Route the `(Binary physical, Utf8 logical)` pair in `schema_adapter.rs` (currently a default Spark `Cast`) through the lossy decoder. ### Options for read-as-Binary 1. **Upstream DataFusion first.** Add a symmetric `string_as_binary` read coercion (mirror of `binary_as_string`). Comet then keeps the required schema as `Utf8`, sets the flag, and the existing `SparkPhysicalExprAdapter` inserts the `Binary -> Utf8` cast at the scan boundary that we route through `decode_utf8_spark_lossy`. Cleanest layering; depends on a DF release or pinning DF to a git rev. 2. **Comet-local reader-factory hook.** Inject an `ARROW:schema` forcing `Binary` and carry `Binary` through the logical schema so the coercion does not undo it. Single Comet PR, but fights the coercion and is fragile around nested types. 3. **Comet-local custom `FileOpener`/`FileSource`.** Fork the `ParquetOpener` to control `ArrowReaderOptions::with_schema` directly. Full control, but duplicates predicate pushdown, page-index, encryption, and metadata-cache logic. Option 1 seems like the right home for the mechanism. Flagging here so we can decide the direction before the Gap A PR. -- 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]
