rich7420 commented on code in PR #6246: URL: https://github.com/apache/datafusion-comet/pull/6246#discussion_r4349097021
########## docs/source/user-guide/latest/parquet.md: ########## @@ -264,11 +270,14 @@ for production, so do not turn it off to avoid an entry on this page. and 4.0, and to Spark 4.1 and later with `spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing=true` ([#7030](https://github.com/apache/datafusion-comet/issues/7030)). -- **Nested BINARY values that are not valid UTF-8, read as STRING.** This applies to a BINARY - column without the UTF8 annotation that is read as `string`, through the read schema or +- **BINARY values that are not valid UTF-8, read as STRING.** This applies to a BINARY column + without the UTF8 annotation that is read as `string`, through the read schema or `spark.sql.parquet.binaryAsString=true`. Spark returns the bytes. Comet returns NULL for each - such nested value. Top-level values match Spark in a release build, because the invalid bytes - reach an Arrow string array unchecked. A debug build panics on them + such nested value. At the top level, `spark.sql.parquet.binaryAsString=true` replaces each + ill-formed sequence with `U+FFFD`, as described in + [Strings with non-UTF-8 bytes](compatibility/index.md#strings-with-non-utf-8-bytes). Through a Review Comment: The linked section still says non-UTF-8 strings don't occur in text read from Parquet. Could it mention this `binaryAsString` path too, so the two pages agree? ########## docs/source/contributor-guide/spark_configs_support.md: ########## @@ -50,6 +50,11 @@ The status column uses these values: - Affected expressions: `date_format`, `from_unixtime`, `unix_timestamp`, `to_unix_timestamp`, `to_timestamp`, `to_timestamp_ntz`, `to_date`, `try_to_date` (Spark 4.1+), `try_to_timestamp` (Spark 3.4+) - Spark versions checked: 3.4.3, 3.5.8, 4.0.1, 4.1.2 - Date: 2026-07-18 +- `spark.sql.parquet.binaryAsString` + - Default: `false` + - Status: Native (see notes) Review Comment: nit: the legend doesn't define `Native`. Would `Partial` fit here, given the documented `U+FFFD` differences? ```suggestion - Status: Partial (see notes) ``` ########## native/core/src/parquet/schema_adapter.rs: ########## @@ -1386,6 +1409,80 @@ impl SparkPhysicalExprAdapter { .data() } + /// A `binaryAsString` scan reads string columns as Binary, so for a file that annotates one + /// as a string, the default adapter compares `CAST(s AS BINARY)` in the pushed filters, and + /// pruning can't see through the cast. The cast keeps the bytes, so `CAST(s AS BINARY) = X'..'` + /// holds exactly when `s` equals the same bytes in its own type, and likewise for `IN` and + /// `IS [NOT] NULL`. Compare the column directly so the file can still prune. + fn unwrap_cast_to_binary_comparison( Review Comment: Could we add tests for `NOT IN`, and for literals this leaves unchanged (invalid UTF-8 or null binary)? I don't think those cases are covered yet. -- 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]
