From 11b5cec4e93a44135f23d9423b173cdd5b3f668d Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 16 Oct 2026 12:22:38 +0800 Subject: [PATCH 01/10] fix: support binaryAsString in the native Parquet scan When spark.sql.parquet.binaryAsString is enabled, Spark infers unannotated Parquet BINARY columns as StringType. arrow-rs validates UTF-8 only for columns annotated as strings, so decoding such a column straight into Arrow Utf8 produced an invalid string array in release builds, which is undefined behavior, and panicked in debug builds. Read top-level string columns as Binary, then decode them to Utf8 at the scan boundary with the same Spark-compatible replacement the cast (#4763) and the FFI import boundary (#5310) use. Nested values are handled separately, in Parquet filter pushdown is skipped while the configuration is enabled, because filter expressions are built against the output Utf8 schema and are not yet rewritten against the Binary read schema. Spark's Filter above the scan re-evaluates every data filter, so results stay correct. Co-Authored-By: Claude Opus 5.5 --- .../spark_configs_support.md | 37 ++++++ native/core/src/execution/delta_dv.rs | 1 + .../operators/dynamic_filter/join/tests.rs | 2 + .../join/tests/schema_errors.rs | 1 + .../tests/schema_errors/partition_columns.rs | 1 + .../join/tests/timestamp_errors.rs | 1 + .../operators/dynamic_filter/topk/tests.rs | 1 + .../dynamic_filter/topk/tests/timestamp.rs | 1 + native/core/src/execution/planner.rs | 1 + native/core/src/parquet/parquet_exec.rs | 113 ++++++++++++++++-- .../src/parquet/parquet_exec/variant_tests.rs | 3 + native/proto/src/proto/operator.proto | 4 + .../serde/operator/CometNativeScan.scala | 3 + .../comet/parquet/ParquetReadSuite.scala | 49 ++++++++ 14 files changed, 210 insertions(+), 8 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 92107975baf..da2553fd062 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/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) + - Affected operator: native Parquet scan + - Date: 2026-09-26 ## Audit Notes @@ -151,3 +156,35 @@ Comet bugs were uncovered by the original audit. The `try_to_timestamp` policy t use the default `query` mode to enforce both result correctness and execution within the Comet pipeline. Tests for expressions with known fallback paths continue to use `query spark_answer_only`. + +### `spark.sql.parquet.binaryAsString` + +**Source.** When enabled, Spark infers unannotated Parquet `BINARY` columns as +`StringType`. Parquet files written by Spark carry Spark schema metadata, so this +configuration does not change the types inferred for those files. + +**Comet status.** Comet honors the configuration in the native Parquet scan. When it is +enabled, unannotated Parquet `BINARY` columns inferred by Spark as `StringType` are read +as Arrow `Binary` and decoded to valid UTF-8 at the scan boundary using Spark-compatible +replacement semantics. arrow-rs validates UTF-8 only for Parquet columns annotated as +strings, so decoding these columns straight into Arrow `Utf8` would put invalid UTF-8 into a +string array, which is undefined behavior. The default `false` value continues to return +unannotated `BINARY` columns as `BinaryType`. + +**Test coverage.** `ParquetReadV1Suite` writes a raw Parquet file without Spark schema +metadata and verifies the inferred schema under both configuration values. It also +asserts that both values use `CometNativeScanExec`, including a row containing invalid +UTF-8 bytes and a projection that reads only a non-string column. + +**Known divergences.** Decoding is not byte-preserving, so the differences described in +[Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any +malformed value this configuration surfaces: byte-level round trips, `octet_length`, +hashing, and value identity, where two distinct malformed sequences both become `U+FFFD`. +This is the same trade-off taken by `CAST(binary AS string)` (#4763) and by the FFI import +boundary (#5310). + +**Known limitation.** Parquet filter pushdown is skipped while this configuration is +enabled, because filter expressions are built against the output `Utf8` schema and are not +yet rewritten against the `Binary` read schema. Results stay correct because Spark's +`Filter` above the scan re-evaluates every data filter, but a scan that would otherwise +prune row groups does not. diff --git a/native/core/src/execution/delta_dv.rs b/native/core/src/execution/delta_dv.rs index a99409d2ed1..742d51f0efd 100644 --- a/native/core/src/execution/delta_dv.rs +++ b/native/core/src/execution/delta_dv.rs @@ -1585,6 +1585,7 @@ mod tests { false, false, false, + false, &session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/join/tests.rs b/native/core/src/execution/operators/dynamic_filter/join/tests.rs index 54ae6737edf..e421acb0764 100644 --- a/native/core/src/execution/operators/dynamic_filter/join/tests.rs +++ b/native/core/src/execution/operators/dynamic_filter/join/tests.rs @@ -666,6 +666,7 @@ fn parquet_probe( false, false, false, + false, session, false, false, @@ -820,6 +821,7 @@ async fn reader_filter_crosses_null_check_conjunction_and_retains_residual() { false, false, false, + false, &session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors.rs b/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors.rs index 8734bca6e29..7748014e7e4 100644 --- a/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors.rs +++ b/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors.rs @@ -82,6 +82,7 @@ fn scan( false, allow_type_promotion, false, + false, session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors/partition_columns.rs b/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors/partition_columns.rs index f6b00114f10..dbe4c077f46 100644 --- a/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors/partition_columns.rs +++ b/native/core/src/execution/operators/dynamic_filter/join/tests/schema_errors/partition_columns.rs @@ -67,6 +67,7 @@ fn partitioned_scan( false, false, false, + false, session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/join/tests/timestamp_errors.rs b/native/core/src/execution/operators/dynamic_filter/join/tests/timestamp_errors.rs index aa982280517..dda22b43456 100644 --- a/native/core/src/execution/operators/dynamic_filter/join/tests/timestamp_errors.rs +++ b/native/core/src/execution/operators/dynamic_filter/join/tests/timestamp_errors.rs @@ -101,6 +101,7 @@ async fn assert_timestamp_overflow_preserved(nested: bool) { false, false, false, + false, &session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/topk/tests.rs b/native/core/src/execution/operators/dynamic_filter/topk/tests.rs index 58a9866bca0..991f81a46d0 100644 --- a/native/core/src/execution/operators/dynamic_filter/topk/tests.rs +++ b/native/core/src/execution/operators/dynamic_filter/topk/tests.rs @@ -192,6 +192,7 @@ fn parquet_scan( false, allow_type_promotion, false, + false, session, false, false, diff --git a/native/core/src/execution/operators/dynamic_filter/topk/tests/timestamp.rs b/native/core/src/execution/operators/dynamic_filter/topk/tests/timestamp.rs index ed5b7267e0a..26c824a0713 100644 --- a/native/core/src/execution/operators/dynamic_filter/topk/tests/timestamp.rs +++ b/native/core/src/execution/operators/dynamic_filter/topk/tests/timestamp.rs @@ -91,6 +91,7 @@ fn timestamp_input( false, false, false, + false, session, false, false, diff --git a/native/core/src/execution/planner.rs b/native/core/src/execution/planner.rs index 8ea033d11da..99bde352b38 100644 --- a/native/core/src/execution/planner.rs +++ b/native/core/src/execution/planner.rs @@ -607,6 +607,7 @@ impl PhysicalPlanner { common.return_null_struct_if_all_fields_missing, common.allow_type_promotion, common.allow_timestamp_ltz_to_ntz, + common.binary_as_string, self.session_ctx(), common.encryption_enabled, common.use_field_id, diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index f2e3e1e4b6c..f297152e3a2 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -24,7 +24,7 @@ use crate::parquet::parquet_support::{ object_store_authority, ObjectStoreBackend, SparkParquetOptions, }; use crate::parquet::schema_adapter::SparkPhysicalExprAdapterFactory; -use arrow::datatypes::{Field, FieldRef, Schema, SchemaRef}; +use arrow::datatypes::{DataType, Field, FieldRef, Schema, SchemaRef}; use datafusion::common::tree_node::TreeNode; use datafusion::config::{ParquetOptions, TableParquetOptions}; use datafusion::datasource::listing::PartitionedFile; @@ -34,12 +34,13 @@ use datafusion::datasource::physical_plan::{ use datafusion::datasource::source::DataSourceExec; use datafusion::execution::object_store::ObjectStoreUrl; use datafusion::physical_expr::expressions::Column; +use datafusion::physical_expr::projection::{ProjectionExpr, ProjectionExprs}; use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_expr_adapter::PhysicalExprAdapterFactory; use datafusion::prelude::SessionContext; use datafusion::scalar::ScalarValue; use datafusion_comet_spark_expr::jvm_udf::JvmScalarUdfExpr; -use datafusion_comet_spark_expr::EvalMode; +use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions}; use datafusion_datasource::TableSchema; use parquet::variant::VariantType; use std::collections::HashMap; @@ -64,6 +65,62 @@ pub(crate) struct CometParquetScanOptions { pub prefetch_next_file_metadata: bool, } +/// Keep top-level Spark string fields as binary while arrow-rs decodes Parquet. arrow-rs only +/// validates UTF-8 for columns annotated as strings, so decoding an unannotated BINARY column that +/// `spark.sql.parquet.binaryAsString=true` types as a string straight into Arrow Utf8 would admit +/// invalid UTF-8. A projection added at the scan boundary converts the binary arrays to valid Utf8 +/// with Spark-compatible replacement semantics. +fn binary_as_string_read_schema(schema: &SchemaRef) -> SchemaRef { + let fields = schema.fields().iter().map(|field| { + let read_type = match field.data_type() { + DataType::Utf8 => Some(DataType::Binary), + DataType::LargeUtf8 => Some(DataType::LargeBinary), + _ => None, + }; + read_type.map_or_else( + || Arc::clone(field), + |data_type| Arc::new(field.as_ref().clone().with_data_type(data_type)), + ) + }); + Arc::new(Schema::new_with_metadata( + fields.collect::>(), + schema.metadata().clone(), + )) +} + +fn binary_as_string_projection( + read_schema: &SchemaRef, + output_schema: &SchemaRef, + projection: Option<&[usize]>, + parquet_options: &SparkParquetOptions, +) -> ProjectionExprs { + let indices: Vec = projection + .map(<[usize]>::to_vec) + .unwrap_or_else(|| (0..read_schema.fields().len()).collect()); + + ProjectionExprs::new(indices.into_iter().map(|index| { + let read_field = read_schema.field(index); + let output_field = output_schema.field(index); + let child: Arc = Arc::new(Column::new(read_field.name().as_str(), index)); + let expr = if read_field.data_type() == output_field.data_type() { + child + } else { + let mut cast_options = + SparkCastOptions::new(parquet_options.eval_mode, &parquet_options.timezone); + cast_options.allow_cast_unsigned_ints = parquet_options.allow_cast_unsigned_ints; + cast_options.is_adapting_schema = true; + Arc::new(Cast::new( + child, + output_field.data_type().clone(), + cast_options, + None, + None, + )) as Arc + }; + ProjectionExpr::new(expr, output_field.name()) + })) +} + /// Initializes a DataSourceExec plan with a ParquetSource for Comet's native Parquet scan. /// /// `required_schema`: Schema to be projected by the scan. @@ -99,6 +156,7 @@ pub(crate) fn init_datasource_exec( return_null_struct_if_all_fields_missing: bool, allow_type_promotion: bool, allow_timestamp_ltz_to_ntz: bool, + binary_as_string: bool, session_ctx: &Arc, encryption_enabled: bool, use_field_id: bool, @@ -198,10 +256,27 @@ pub(crate) fn init_datasource_exec( .flat_map(|s| s.fields().iter()) .map(|f| Arc::new(Field::new(f.name(), f.data_type().clone(), f.is_nullable()))) .collect(); - let table_schema = TableSchema::builder(base_schema) + let output_table_schema = TableSchema::builder(Arc::clone(&base_schema)) + .with_table_partition_cols(partition_fields.clone()) + .build(); + let read_base_schema = if binary_as_string { + binary_as_string_read_schema(&base_schema) + } else { + Arc::clone(&base_schema) + }; + let table_schema = TableSchema::builder(read_base_schema) .with_table_partition_cols(partition_fields) .build(); + let binary_projection = binary_as_string.then(|| { + binary_as_string_projection( + table_schema.table_schema(), + output_table_schema.table_schema(), + projection.as_deref(), + &spark_parquet_options, + ) + }); + let mut parquet_source = ParquetSource::new(table_schema) .with_table_parquet_options(table_parquet_options) .with_metadata_size_hint(METADATA_SIZE_HINT); @@ -298,8 +373,11 @@ pub(crate) fn init_datasource_exec( .filter(|filter| !calls_jvm(filter)) .collect::>() }); - let file_source: Arc = match data_filters { - Some(filters) if !filters.is_empty() => { + let mut file_source: Arc = match data_filters { + // The Spark Filter above the scan re-evaluates every dataFilter. Until filter expressions + // are rewritten from the output Utf8 schema to the Binary read schema, skip only the + // optional Parquet pushdown in binaryAsString mode. + Some(filters) if !filters.is_empty() && !binary_as_string => { let propagation = parquet_source.try_pushdown_filters(filters, session_config.options())?; // `updated_node` is `None` when every filter classified as `No` @@ -311,6 +389,16 @@ pub(crate) fn init_datasource_exec( _ => Arc::new(parquet_source), }; + if let Some(binary_projection) = &binary_projection { + file_source = file_source + .try_pushdown_projection(binary_projection)? + .ok_or_else(|| { + ExecutionError::GeneralError( + "ParquetSource rejected binaryAsString projection pushdown".to_string(), + ) + })?; + } + let file_source = if spark_parquet_options.checked_timestamp_overflow { ParquetErrorContext::wrap(file_source) } else { @@ -329,9 +417,11 @@ pub(crate) fn init_datasource_exec( .with_file_groups(file_groups) .with_expr_adapter(Some(expr_adapter_factory)); - if let Some(projection) = projection { - file_scan_config_builder = - file_scan_config_builder.with_projection_indices(Some(projection))?; + if binary_projection.is_none() { + if let Some(projection) = projection { + file_scan_config_builder = + file_scan_config_builder.with_projection_indices(Some(projection))?; + } } let file_scan_config = file_scan_config_builder.build(); @@ -571,6 +661,7 @@ mod tests { false, false, false, + false, session_ctx, false, false, @@ -708,6 +799,7 @@ mod tests { false, false, false, + false, &session_ctx, false, false, @@ -784,6 +876,7 @@ mod tests { false, false, false, + false, &session_ctx, false, false, @@ -893,6 +986,7 @@ mod tests { false, false, false, + false, &session_ctx, false, false, @@ -989,6 +1083,7 @@ mod tests { false, false, false, + false, &session_ctx, false, false, @@ -1136,6 +1231,7 @@ mod tests { false, false, allow_timestamp_ltz_to_ntz, + false, &session_ctx, false, false, @@ -1405,6 +1501,7 @@ mod tests { false, false, false, + false, &session_ctx, false, false, diff --git a/native/core/src/parquet/parquet_exec/variant_tests.rs b/native/core/src/parquet/parquet_exec/variant_tests.rs index 0bfdcecd222..61a26fa9856 100644 --- a/native/core/src/parquet/parquet_exec/variant_tests.rs +++ b/native/core/src/parquet/parquet_exec/variant_tests.rs @@ -159,6 +159,7 @@ async fn scan_variant_file(filename: PathBuf) -> VariantArray { false, false, false, + false, &session_ctx, false, false, @@ -227,6 +228,7 @@ async fn unread_variant_does_not_override_arrow_schema_hint() { false, false, false, + false, &session, false, false, @@ -260,6 +262,7 @@ fn encrypted_projected_variant_is_rejected_before_reader_creation() { false, false, false, + false, &session, true, false, diff --git a/native/proto/src/proto/operator.proto b/native/proto/src/proto/operator.proto index 90c01a7511d..c6acd3f73e4 100644 --- a/native/proto/src/proto/operator.proto +++ b/native/proto/src/proto/operator.proto @@ -186,6 +186,10 @@ message NativeScanCommon { // True when Spark supplied a Parquet data filter, including when Comet could // not serialize any of those filters into data_filters. bool has_data_filters = 19; + // Spark infers unannotated Parquet BINARY columns as StringType when + // spark.sql.parquet.binaryAsString is enabled. The native reader keeps those + // values as Binary until a Spark-compatible UTF-8 decode at the scan boundary. + bool binary_as_string = 20; } message NativeScan { diff --git a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala index e8efa37202c..19cdd7aa903 100644 --- a/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala +++ b/spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala @@ -308,6 +308,9 @@ object CometNativeScan extends CometOperatorSerde[CometScanExec] with CometTypeS hadoopConf = hadoopConf, conf = scan.conf) match { case Some(commonBuilder) => + // Only Spark's own Parquet schema inference reads this flag, so it is set here rather + // than in populateScanConfFlags, which contrib scans with their own schemas share. + commonBuilder.setBinaryAsString(scan.conf.getConf(SQLConf.PARQUET_BINARY_AS_STRING)) // Sink operators don't have children builder.clearChildren() val nativeScanBuilder = OperatorOuterClass.NativeScan.newBuilder() diff --git a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala index 56743367a4a..f6c1ad918d4 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -309,6 +309,55 @@ abstract class ParquetReadSuite extends CometTestBase { } } + test("spark.sql.parquet.binaryAsString uses native scan") { + withTempDir { dir => + val path = new Path(dir.toURI.toString, "part-r-0.parquet") + val schema = MessageTypeParser.parseMessageType(""" + |message root { + | optional binary value; + | optional int32 id; + |} + |""".stripMargin) + val writer = createParquetWriter(schema, path) + Seq(Binary.fromString("one"), Binary.fromConstantByteArray(Array(0xff.toByte))).zipWithIndex + .foreach { case (value, id) => + val record = new SimpleGroup(schema) + record.add(0, value) + record.add(1, id) + writer.write(record) + } + writer.close() + + withSQLConf(SQLConf.PARQUET_BINARY_AS_STRING.key -> "false") { + val df = spark.read.parquet(path.toString) + assert( + df.schema === StructType( + Seq(StructField("value", BinaryType, true), StructField("id", IntegerType, true)))) + val (_, cometPlan) = checkSparkAnswer(df) + assert( + collect(cometPlan) { case _: CometNativeScanExec => true }.nonEmpty, + s"Expected native scan with binaryAsString disabled:\n${cometPlan.treeString}") + } + + withSQLConf(SQLConf.PARQUET_BINARY_AS_STRING.key -> "true") { + val df = spark.read.parquet(path.toString) + assert( + df.schema === StructType( + Seq(StructField("value", StringType, true), StructField("id", IntegerType, true)))) + val (_, cometPlan) = checkSparkAnswer(df) + assert( + collect(cometPlan) { case _: CometNativeScanExec => true }.nonEmpty, + s"Expected native scan with binaryAsString enabled:\n${cometPlan.treeString}") + + val projected = spark.read.parquet(path.toString).select("id") + val (_, projectedCometPlan) = checkSparkAnswer(projected) + assert( + collect(projectedCometPlan) { case _: CometNativeScanExec => true }.nonEmpty, + s"Expected native scan when no string columns are read:\n${projectedCometPlan.treeString}") + } + } + } + test("string") { val data = (1 to 4).map(i => Tuple1(i.toString)) // Property spark.sql.parquet.binaryAsString shouldn't affect Parquet files written by Spark SQL From 4ba3f9c31c79bf9043c067b8daaa7b47e59d617c Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 16 Oct 2026 12:25:27 +0800 Subject: [PATCH 02/10] perf: push binaryAsString scan filters that don't read a string column With spark.sql.parquet.binaryAsString enabled, the native scan skipped Parquet filter pushdown for every filter, because filters are typed against the Utf8 output schema while top-level string columns are read as Binary. Only filters on those columns are affected, so drop just those and push the rest, which still prune row groups. Spark's Filter above the scan still evaluates every data filter. Co-Authored-By: Claude Opus 5.5 --- .../spark_configs_support.md | 17 +- native/core/src/parquet/parquet_exec.rs | 180 ++++++++++++++++-- 2 files changed, 177 insertions(+), 20 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index da2553fd062..4c7e574e196 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -54,7 +54,7 @@ The status column uses these values: - Default: `false` - Status: Native (see notes) - Affected operator: native Parquet scan - - Date: 2026-09-26 + - Date: 2026-10-16 ## Audit Notes @@ -174,7 +174,9 @@ unannotated `BINARY` columns as `BinaryType`. **Test coverage.** `ParquetReadV1Suite` writes a raw Parquet file without Spark schema metadata and verifies the inferred schema under both configuration values. It also asserts that both values use `CometNativeScanExec`, including a row containing invalid -UTF-8 bytes and a projection that reads only a non-string column. +UTF-8 bytes and a projection that reads only a non-string column. A native test in +`parquet_exec.rs` checks that a filter on a non-string column still prunes row groups when +it is combined with a filter on a string column. **Known divergences.** Decoding is not byte-preserving, so the differences described in [Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any @@ -183,8 +185,9 @@ hashing, and value identity, where two distinct malformed sequences both become This is the same trade-off taken by `CAST(binary AS string)` (#4763) and by the FFI import boundary (#5310). -**Known limitation.** Parquet filter pushdown is skipped while this configuration is -enabled, because filter expressions are built against the output `Utf8` schema and are not -yet rewritten against the `Binary` read schema. Results stay correct because Spark's -`Filter` above the scan re-evaluates every data filter, but a scan that would otherwise -prune row groups does not. +**Known limitation.** While this configuration is enabled, data filters that reference a +top-level string column are not pushed into the Parquet scan, because filter expressions +are built against the output `Utf8` schema and are not yet rewritten against the `Binary` +read schema. Filters on other columns are still pushed down. Results stay correct because +Spark's `Filter` above the scan re-evaluates every data filter, but a filter on a string +column does not prune row groups. diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index f297152e3a2..3e529f3354c 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -19,7 +19,7 @@ use crate::execution::operators::ExecutionError; use crate::parquet::eager_page_index_reader_factory::{EagerPageIndexReaderFactory, ScanIoSource}; use crate::parquet::encryption_support::{CometEncryptionConfig, ENCRYPTION_FACTORY_ID}; use crate::parquet::file_error_context::ParquetErrorContext; -use crate::parquet::name_fold::fold_schema_names; +use crate::parquet::name_fold::{fold_name, fold_names, fold_schema_names}; use crate::parquet::parquet_support::{ object_store_authority, ObjectStoreBackend, SparkParquetOptions, }; @@ -35,6 +35,7 @@ use datafusion::datasource::source::DataSourceExec; use datafusion::execution::object_store::ObjectStoreUrl; use datafusion::physical_expr::expressions::Column; use datafusion::physical_expr::projection::{ProjectionExpr, ProjectionExprs}; +use datafusion::physical_expr::utils::collect_columns; use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_expr_adapter::PhysicalExprAdapterFactory; use datafusion::prelude::SessionContext; @@ -43,7 +44,7 @@ use datafusion_comet_spark_expr::jvm_udf::JvmScalarUdfExpr; use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions}; use datafusion_datasource::TableSchema; use parquet::variant::VariantType; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; #[cfg(test)] @@ -72,12 +73,7 @@ pub(crate) struct CometParquetScanOptions { /// with Spark-compatible replacement semantics. fn binary_as_string_read_schema(schema: &SchemaRef) -> SchemaRef { let fields = schema.fields().iter().map(|field| { - let read_type = match field.data_type() { - DataType::Utf8 => Some(DataType::Binary), - DataType::LargeUtf8 => Some(DataType::LargeBinary), - _ => None, - }; - read_type.map_or_else( + binary_as_string_read_type(field.data_type()).map_or_else( || Arc::clone(field), |data_type| Arc::new(field.as_ref().clone().with_data_type(data_type)), ) @@ -88,6 +84,30 @@ fn binary_as_string_read_schema(schema: &SchemaRef) -> SchemaRef { )) } +/// The type `binary_as_string_read_schema` reads a top-level field of `data_type` as, or `None` +/// when the field is read as declared. +fn binary_as_string_read_type(data_type: &DataType) -> Option { + match data_type { + DataType::Utf8 => Some(DataType::Binary), + DataType::LargeUtf8 => Some(DataType::LargeBinary), + _ => None, + } +} + +/// Whether `expr` references a column whose folded name is in `folded_names`. +fn references_any_column( + expr: &Arc, + folded_names: &HashSet, + case_sensitive: bool, +) -> Result { + for column in collect_columns(expr) { + if folded_names.contains(&fold_name(column.name(), case_sensitive)?) { + return Ok(true); + } + } + Ok(false) +} + fn binary_as_string_projection( read_schema: &SchemaRef, output_schema: &SchemaRef, @@ -268,6 +288,20 @@ pub(crate) fn init_datasource_exec( .with_table_partition_cols(partition_fields) .build(); + // Folded names of the columns read as binary. Filters that reference one are kept out of the + // scan below. + let binary_read_columns: HashSet = if binary_as_string { + let names: Vec<&str> = base_schema + .fields() + .iter() + .filter(|field| binary_as_string_read_type(field.data_type()).is_some()) + .map(|field| field.name().as_str()) + .collect(); + fold_names(&names, case_sensitive)?.into_iter().collect() + } else { + HashSet::new() + }; + let binary_projection = binary_as_string.then(|| { binary_as_string_projection( table_schema.table_schema(), @@ -373,11 +407,23 @@ pub(crate) fn init_datasource_exec( .filter(|filter| !calls_jvm(filter)) .collect::>() }); + // In binaryAsString mode the filters are typed against the Utf8 output schema, but the scan + // reads the string columns as Binary, so drop the filters that reference one for the same + // reason. Filters on other columns still prune. + let data_filters = match data_filters { + Some(filters) if !binary_read_columns.is_empty() => { + let mut pushable = Vec::with_capacity(filters.len()); + for filter in filters { + if !references_any_column(&filter, &binary_read_columns, case_sensitive)? { + pushable.push(filter); + } + } + Some(pushable) + } + other => other, + }; let mut file_source: Arc = match data_filters { - // The Spark Filter above the scan re-evaluates every dataFilter. Until filter expressions - // are rewritten from the output Utf8 schema to the Binary read schema, skip only the - // optional Parquet pushdown in binaryAsString mode. - Some(filters) if !filters.is_empty() && !binary_as_string => { + Some(filters) if !filters.is_empty() => { let propagation = parquet_source.try_pushdown_filters(filters, session_config.options())?; // `updated_node` is `None` when every filter classified as `No` @@ -546,7 +592,7 @@ fn get_options( #[cfg(test)] mod tests { use super::*; - use arrow::array::{Date32Array, Int32Array}; + use arrow::array::{AsArray, BinaryArray, Date32Array, Int32Array}; use arrow::datatypes::{DataType, Field, Schema}; use arrow::record_batch::RecordBatch; use bytes::Bytes; @@ -596,6 +642,114 @@ mod tests { assert!(!calls_jvm(&native)); } + #[tokio::test] + async fn binary_as_string_prunes_with_filters_on_other_columns() { + let file_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, false), + Field::new("s", DataType::Binary, true), + ])); + let filename = get_temp_filename() + .as_path() + .as_os_str() + .to_str() + .unwrap() + .to_string(); + let props = WriterProperties::builder() + .set_max_row_group_row_count(Some(500)) + .build(); + let file = File::create(&filename).unwrap(); + let mut writer = ArrowWriter::try_new(file, Arc::clone(&file_schema), Some(props)).unwrap(); + let invalid_utf8: &[u8] = &[0xff]; + for start in [0, 1_000] { + let batch = RecordBatch::try_new( + Arc::clone(&file_schema), + vec![ + Arc::new(Int32Array::from((start..start + 500).collect::>())), + Arc::new(BinaryArray::from_iter_values( + (0..500).map(|_| invalid_utf8), + )), + ], + ) + .unwrap(); + writer.write(&batch).unwrap(); + } + writer.close().unwrap(); + + let partitioned_file = PartitionedFile::from_path(filename).unwrap(); + let session_ctx = Arc::new(SessionContext::new()); + let required_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, false), + Field::new("s", DataType::Utf8, true), + ])); + let scan = |filters: Option>>| { + init_datasource_exec( + Arc::clone(&required_schema), + Some(Arc::clone(&required_schema)), + None, + ObjectStoreUrl::local_filesystem(), + ObjectStoreBackend::Local, + vec![vec![partitioned_file.clone()]], + Some(vec![0, 1]), + filters, + None, + "UTC", + true, + false, + false, + false, + true, + &session_ctx, + false, + false, + false, + false, + "", + "", + ) + .unwrap() + }; + + let full_scan = scan(None); + drain_scan(&full_scan, &session_ctx).await; + + let a_below_500: Arc = Arc::new(BinaryExpr::new( + Arc::new(Column::new("a", 0)), + Operator::Lt, + Arc::new(Literal::new(ScalarValue::Int32(Some(500)))), + )); + let s_equals: Arc = Arc::new(BinaryExpr::new( + Arc::new(Column::new("s", 1)), + Operator::Eq, + Arc::new(Literal::new(ScalarValue::Utf8(Some("x".to_string())))), + )); + let filtered_scan = scan(Some(vec![a_below_500, s_equals])); + let batches: Vec = filtered_scan + .execute(0, session_ctx.task_ctx()) + .unwrap() + .map(|batch| batch.unwrap()) + .collect() + .await; + + assert_eq!( + batches.iter().map(RecordBatch::num_rows).sum::(), + 500 + ); + for batch in &batches { + assert_eq!(batch.schema().field(1).data_type(), &DataType::Utf8); + assert!(batch + .column(1) + .as_string::() + .iter() + .all(|value| value == Some("\u{fffd}"))); + } + let full_data = scan_metric(&full_scan, "scan_io_data_bytes"); + let filtered_data = scan_metric(&filtered_scan, "scan_io_data_bytes"); + assert!( + filtered_data < full_data, + "the filter on `a` should prune a row group: filtered={filtered_data}, full={full_data}" + ); + } + fn write_scan_io_fixture() -> (String, SchemaRef) { let schema = Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, false), From 69f74e0c8be09cb0457e604c0735452169f3882d Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 21 Oct 2026 00:18:47 +0800 Subject: [PATCH 03/10] perf: push binaryAsString equality and null filters as raw-byte filters With spark.sql.parquet.binaryAsString enabled, the scan reads top-level string columns as Binary, so filters on them were kept out of the scan and pruned nothing. Push =, IN and IS [NOT] NULL on those columns with a Binary literal instead. A valid value decodes to itself and a malformed one to a string containing U+FFFD, so for a literal without U+FFFD the byte comparison keeps exactly the rows the string comparison keeps. Ranges, LIKE, function calls and literals containing U+FFFD stay above the scan, because decoding can reorder values. An AND keeps whichever side can be pushed, since Spark's Filter above the scan re-evaluates every data filter. For a file that annotates the column as a string, the default adapter compares CAST(s AS BINARY), which pruning can't see through. Comet's adapter now compares the column with the literal in the column's own type, so those files prune too. A join or TopK dynamic filter is pushed onto the scan's output, so it is mapped through the decoding projection and compares decoded strings. A test pins that, though today these filters accept only integer keys. Also read only Utf8 fields as Binary. Spark's StringType is never LargeUtf8, and spark_cast has no LargeBinary to LargeUtf8 arm. Co-Authored-By: Claude Opus 5.5 --- .../spark_configs_support.md | 57 ++- docs/source/user-guide/latest/parquet.md | 18 +- native/core/src/parquet/parquet_exec.rs | 474 ++++++++++++++---- native/core/src/parquet/schema_adapter.rs | 99 +++- .../comet/parquet/ParquetReadSuite.scala | 76 +++ 5 files changed, 600 insertions(+), 124 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 4c7e574e196..39d69c3d067 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -54,7 +54,7 @@ The status column uses these values: - Default: `false` - Status: Native (see notes) - Affected operator: native Parquet scan - - Date: 2026-10-16 + - Date: 2026-10-21 ## Audit Notes @@ -164,19 +164,41 @@ the Comet pipeline. Tests for expressions with known fallback paths continue to configuration does not change the types inferred for those files. **Comet status.** Comet honors the configuration in the native Parquet scan. When it is -enabled, unannotated Parquet `BINARY` columns inferred by Spark as `StringType` are read -as Arrow `Binary` and decoded to valid UTF-8 at the scan boundary using Spark-compatible -replacement semantics. arrow-rs validates UTF-8 only for Parquet columns annotated as -strings, so decoding these columns straight into Arrow `Utf8` would put invalid UTF-8 into a -string array, which is undefined behavior. The default `false` value continues to return -unannotated `BINARY` columns as `BinaryType`. - -**Test coverage.** `ParquetReadV1Suite` writes a raw Parquet file without Spark schema -metadata and verifies the inferred schema under both configuration values. It also -asserts that both values use `CometNativeScanExec`, including a row containing invalid -UTF-8 bytes and a projection that reads only a non-string column. A native test in -`parquet_exec.rs` checks that a filter on a non-string column still prunes row groups when -it is combined with a filter on a string column. +enabled, the scan reads every top-level string column as Arrow `Binary` and decodes it to valid +UTF-8 at the scan boundary using Spark-compatible replacement semantics. Spark's schema doesn't +say which file columns carry the UTF8 annotation, so annotated columns take the same path. +arrow-rs validates UTF-8 only for Parquet columns annotated as strings, so decoding unannotated +columns straight into Arrow `Utf8` would put invalid UTF-8 into a string array, which is undefined +behavior. The default `false` value continues to return unannotated `BINARY` columns as +`BinaryType`. + +**Filter pushdown.** While the configuration is enabled, the scan rewrites `=`, `IN` and +`IS [NOT] NULL` on a top-level string column to compare raw bytes, so `value = 'one'` becomes +`value = X'6F6E65'`. A valid value decodes to itself, and a malformed one to a string containing +`U+FFFD`, so for a literal without `U+FFFD` the rewritten filter keeps exactly the rows the +original keeps. For a file that annotates the column as a string, the expression adapter compares +the column with the string literal directly, so the file's statistics still prune. Other filters +on these columns stay above the scan, because decoding can reorder values: `"a\x80"` sorts below +`"aé"` as bytes, but `"a\uFFFD"` sorts above it. That covers ranges, `LIKE`, function calls and +literals containing `U+FFFD`. Spark's `Filter` above the scan re-evaluates every data filter, so +results don't depend on what is pushed. Join and TopK dynamic filters accept only integer keys, +so they never reach these columns. + +Compared with Spark, Comet prunes more on unannotated columns and less on annotated ones. Spark +pushes a filter only when the literal's type matches the column's Parquet type, so it doesn't push +`=` or `IN` on an unannotated `BINARY` column, though it does push `IS NULL`. On an annotated +column Spark also pushes ranges, which Comet keeps above the scan while the configuration is on. + +**Test coverage.** `ParquetReadV1Suite` writes raw Parquet files without Spark schema metadata +and verifies the inferred schema under both configuration values, Spark answer parity and +`CometNativeScanExec`, including a row containing invalid UTF-8 bytes and a projection that reads +only a non-string column. A second test reads two partition directories holding a valid value, +`0xff` and a null, and a Spark-written file, with row-filter pushdown off and on. It compares `=`, +`IN`, `IS [NOT] NULL`, a range, an `OR`, a self-join on the string column and an +`ORDER BY ... LIMIT` against Spark, and checks that `value = 'one'` reaches the row filter. Native +tests in `parquet_exec.rs` check which row groups each filter prunes, for an unannotated and an +annotated column, with and without row filters. They include a range and a `U+FFFD` literal, +neither of which may prune. **Known divergences.** Decoding is not byte-preserving, so the differences described in [Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any @@ -184,10 +206,3 @@ malformed value this configuration surfaces: byte-level round trips, `octet_leng hashing, and value identity, where two distinct malformed sequences both become `U+FFFD`. This is the same trade-off taken by `CAST(binary AS string)` (#4763) and by the FFI import boundary (#5310). - -**Known limitation.** While this configuration is enabled, data filters that reference a -top-level string column are not pushed into the Parquet scan, because filter expressions -are built against the output `Utf8` schema and are not yet rewritten against the `Binary` -read schema. Filters on other columns are still pushed down. Results stay correct because -Spark's `Filter` above the scan re-evaluates every data filter, but a filter on a string -column does not prune row groups. diff --git a/docs/source/user-guide/latest/parquet.md b/docs/source/user-guide/latest/parquet.md index 22b8fe71a4d..466ed37c369 100644 --- a/docs/source/user-guide/latest/parquet.md +++ b/docs/source/user-guide/latest/parquet.md @@ -211,6 +211,12 @@ page-index, and bloom-filter pruning happen regardless of this flag whenever Spa `spark.sql.parquet.filterPushdown` is on, except for filters that compare `FLOAT` or `DOUBLE` values, which do not prune while this flag is on (see [Floating-point Number Comparison](compatibility/floating-point.md)). +With `spark.sql.parquet.binaryAsString=true`, the native scan reads every top-level string column +as binary and decodes it after reading. Filters on those columns prune only for `=`, `IN` and +`IS [NOT] NULL` against a literal without `U+FFFD`, which the scan compares as raw bytes. Other +filters on them, such as ranges and `LIKE`, don't prune while the setting is on, even for columns +the file annotates as strings. + ### Row Group Assignment to Splits Spark and DataFusion's native Parquet scans use different rules to decide which row groups belong to a @@ -291,11 +297,13 @@ for production, so do not turn it off to avoid an entry on this page. - **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 keeps the bytes. In a struct field, list element - or map value, Comet 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). Top-level - values, and map keys at any depth, reach an Arrow string array unchecked, which keeps keys that - differ only in such bytes distinct: they match Spark in a release build, and a debug build - panics on top-level values ([#7032](https://github.com/apache/datafusion-comet/issues/7032), part of + or map value, and at the top level with `spark.sql.parquet.binaryAsString=true`, Comet 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 + read schema, top-level values reach an Arrow string array unchecked, as do map keys at any depth, + which keeps keys that differ only in such bytes distinct: they match Spark in a release build, + and a debug build panics on top-level values + ([#7032](https://github.com/apache/datafusion-comet/issues/7032), part of [#4764](https://github.com/apache/datafusion-comet/issues/4764)). - **Field-id reads where one file column matches a field by id and a sibling by name.** With `spark.sql.parquet.fieldId.read.enabled=true`, Comet returns NULL for the sibling at the top diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index 3e529f3354c..e8f9e5a7c00 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -33,7 +33,10 @@ use datafusion::datasource::physical_plan::{ }; use datafusion::datasource::source::DataSourceExec; use datafusion::execution::object_store::ObjectStoreUrl; -use datafusion::physical_expr::expressions::Column; +use datafusion::logical_expr::Operator; +use datafusion::physical_expr::expressions::{ + in_list, BinaryExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, Literal, +}; use datafusion::physical_expr::projection::{ProjectionExpr, ProjectionExprs}; use datafusion::physical_expr::utils::collect_columns; use datafusion::physical_expr::PhysicalExpr; @@ -72,28 +75,31 @@ pub(crate) struct CometParquetScanOptions { /// invalid UTF-8. A projection added at the scan boundary converts the binary arrays to valid Utf8 /// with Spark-compatible replacement semantics. fn binary_as_string_read_schema(schema: &SchemaRef) -> SchemaRef { - let fields = schema.fields().iter().map(|field| { - binary_as_string_read_type(field.data_type()).map_or_else( - || Arc::clone(field), - |data_type| Arc::new(field.as_ref().clone().with_data_type(data_type)), - ) - }); Arc::new(Schema::new_with_metadata( - fields.collect::>(), + schema + .fields() + .iter() + .map(binary_as_string_read_field) + .collect::>(), schema.metadata().clone(), )) } -/// The type `binary_as_string_read_schema` reads a top-level field of `data_type` as, or `None` -/// when the field is read as declared. -fn binary_as_string_read_type(data_type: &DataType) -> Option { - match data_type { - DataType::Utf8 => Some(DataType::Binary), - DataType::LargeUtf8 => Some(DataType::LargeBinary), - _ => None, +/// The field `binary_as_string_read_schema` reads a top-level `field` as. Spark's StringType is +/// Arrow Utf8. +fn binary_as_string_read_field(field: &FieldRef) -> FieldRef { + if is_binary_as_string_read(field.data_type()) { + Arc::new(field.as_ref().clone().with_data_type(DataType::Binary)) + } else { + Arc::clone(field) } } +/// Whether `binary_as_string_read_schema` reads a top-level field of `data_type` as Binary. +fn is_binary_as_string_read(data_type: &DataType) -> bool { + data_type == &DataType::Utf8 +} + /// Whether `expr` references a column whose folded name is in `folded_names`. fn references_any_column( expr: &Arc, @@ -108,6 +114,109 @@ fn references_any_column( Ok(false) } +/// Rewrites a data filter for a scan that reads string columns as Binary (see +/// `binary_as_string_read_schema`), or returns `None` when it can't go into the scan. +/// +/// The scan decodes each such column with replacement: a valid value decodes to itself, and an +/// invalid one to a string containing U+FFFD. So for a literal without U+FFFD, `s = 'lit'` on the +/// decoded string holds exactly when `s = X''` holds on the raw bytes, and the same +/// goes for `IN`. `IS [NOT] NULL` holds on both, because decoding keeps nulls. Other filters on +/// these columns stay above the scan. A range can disagree because decoding can reorder values: +/// `"a\x80"` sorts below `"aé"` as bytes, but `"a\u{FFFD}"` sorts above it. +/// +/// Spark's Filter above the scan re-evaluates every data filter, so a pushed filter only has to +/// keep every row the original keeps. That's why an `AND` keeps whichever side can be pushed. +fn binary_as_string_pushdown_filter( + filter: &Arc, + binary_read_columns: &HashSet, + case_sensitive: bool, + filter_read_schema: &Schema, +) -> Result>, ExecutionError> { + if !references_any_column(filter, binary_read_columns, case_sensitive)? { + return Ok(Some(Arc::clone(filter))); + } + let reads_binary = |expr: &Arc| -> Result { + match expr.downcast_ref::() { + Some(column) => { + Ok(binary_read_columns.contains(&fold_name(column.name(), case_sensitive)?)) + } + None => Ok(false), + } + }; + let pushdown = |expr: &Arc| { + binary_as_string_pushdown_filter( + expr, + binary_read_columns, + case_sensitive, + filter_read_schema, + ) + }; + + if let Some(binary) = filter.downcast_ref::() { + let (left, right) = (binary.left(), binary.right()); + match binary.op() { + Operator::And => { + return Ok(match (pushdown(left)?, pushdown(right)?) { + (Some(left), Some(right)) => { + Some(Arc::new(BinaryExpr::new(left, Operator::And, right))) + } + (side, None) | (None, side) => side, + }); + } + Operator::Or => { + return Ok(match (pushdown(left)?, pushdown(right)?) { + (Some(left), Some(right)) => { + Some(Arc::new(BinaryExpr::new(left, Operator::Or, right))) + } + _ => None, + }); + } + Operator::Eq => { + if reads_binary(left)? { + if let Some(literal) = binary_literal(right) { + let eq = BinaryExpr::new(Arc::clone(left), Operator::Eq, literal); + return Ok(Some(Arc::new(eq))); + } + } + if reads_binary(right)? { + if let Some(literal) = binary_literal(left) { + let eq = BinaryExpr::new(literal, Operator::Eq, Arc::clone(right)); + return Ok(Some(Arc::new(eq))); + } + } + } + _ => {} + } + } else if let Some(list) = filter.downcast_ref::() { + if !list.negated() && reads_binary(list.expr())? { + if let Some(values) = list.list().iter().map(binary_literal).collect() { + let expr = Arc::clone(list.expr()); + return Ok(Some(in_list(expr, values, &false, filter_read_schema)?)); + } + } + } else if let Some(is_null) = filter.downcast_ref::() { + if reads_binary(is_null.arg())? { + return Ok(Some(Arc::clone(filter))); + } + } else if let Some(is_not_null) = filter.downcast_ref::() { + if reads_binary(is_not_null.arg())? { + return Ok(Some(Arc::clone(filter))); + } + } + Ok(None) +} + +/// `expr` as a Binary literal holding the same bytes, if it is a string literal without U+FFFD. +fn binary_literal(expr: &Arc) -> Option> { + match expr.downcast_ref::()?.value() { + ScalarValue::Utf8(Some(value)) if !value.contains(char::REPLACEMENT_CHARACTER) => { + let bytes = value.as_bytes().to_vec(); + Some(Arc::new(Literal::new(ScalarValue::Binary(Some(bytes))))) + } + _ => None, + } +} + fn binary_as_string_projection( read_schema: &SchemaRef, output_schema: &SchemaRef, @@ -285,16 +394,16 @@ pub(crate) fn init_datasource_exec( Arc::clone(&base_schema) }; let table_schema = TableSchema::builder(read_base_schema) - .with_table_partition_cols(partition_fields) + .with_table_partition_cols(partition_fields.clone()) .build(); - // Folded names of the columns read as binary. Filters that reference one are kept out of the - // scan below. + // Folded names of the columns read as binary. Filters that reference one are rewritten for + // the binary read below, or kept out of the scan. let binary_read_columns: HashSet = if binary_as_string { let names: Vec<&str> = base_schema .fields() .iter() - .filter(|field| binary_as_string_read_type(field.data_type()).is_some()) + .filter(|field| is_binary_as_string_read(field.data_type())) .map(|field| field.name().as_str()) .collect(); fold_names(&names, case_sensitive)?.into_iter().collect() @@ -408,13 +517,28 @@ pub(crate) fn init_datasource_exec( .collect::>() }); // In binaryAsString mode the filters are typed against the Utf8 output schema, but the scan - // reads the string columns as Binary, so drop the filters that reference one for the same - // reason. Filters on other columns still prune. + // reads the string columns as Binary. Rewrite the filters on those columns that can compare + // raw bytes, and keep the rest out of the scan. Filters on other columns are pushed as is. let data_filters = match data_filters { Some(filters) if !binary_read_columns.is_empty() => { + // The planner binds data filters against the required columns followed by the + // partition columns. + let filter_read_schema = Schema::new( + required_schema + .fields() + .iter() + .map(binary_as_string_read_field) + .chain(partition_fields.iter().cloned()) + .collect::>(), + ); let mut pushable = Vec::with_capacity(filters.len()); for filter in filters { - if !references_any_column(&filter, &binary_read_columns, case_sensitive)? { + if let Some(filter) = binary_as_string_pushdown_filter( + &filter, + &binary_read_columns, + case_sensitive, + &filter_read_schema, + )? { pushable.push(filter); } } @@ -605,6 +729,7 @@ mod tests { use datafusion::physical_expr::expressions::{BinaryExpr, Literal}; use datafusion::physical_plan::metrics::ExecutionPlanMetricsSet; use datafusion::physical_plan::ExecutionPlan; + use datafusion::prelude::SessionConfig; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; use futures::StreamExt; use object_store::memory::InMemory; @@ -616,6 +741,7 @@ mod tests { use parquet::encryption::encrypt::FileEncryptionProperties; use parquet::file::metadata::{KeyValue, PageIndexPolicy, ParquetMetaDataReader}; use parquet::file::properties::{EnabledStatistics, WriterProperties}; + use std::collections::BTreeMap; use std::fs::File; use std::time::Duration; @@ -642,11 +768,19 @@ mod tests { assert!(!calls_jvm(&native)); } - #[tokio::test] - async fn binary_as_string_prunes_with_filters_on_other_columns() { + /// How many rows of a scan hold each decoded value of `s`. + type DecodedCounts = BTreeMap, usize>; + + /// Writes three 500-row row groups with `a` counting from 0, 1,000 and 2,000, and `s` holding + /// `values[i]` in every row of group `i` as `s_type`. A scan's values then show which groups a + /// filter pruned. + fn write_binary_as_string_fixture( + s_type: DataType, + values: [Option<&[u8]>; 3], + ) -> PartitionedFile { let file_schema = Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, false), - Field::new("s", DataType::Binary, true), + Field::new("s", s_type.clone(), true), ])); let filename = get_temp_filename() .as_path() @@ -659,95 +793,241 @@ mod tests { .build(); let file = File::create(&filename).unwrap(); let mut writer = ArrowWriter::try_new(file, Arc::clone(&file_schema), Some(props)).unwrap(); - let invalid_utf8: &[u8] = &[0xff]; - for start in [0, 1_000] { - let batch = RecordBatch::try_new( - Arc::clone(&file_schema), - vec![ - Arc::new(Int32Array::from((start..start + 500).collect::>())), - Arc::new(BinaryArray::from_iter_values( - (0..500).map(|_| invalid_utf8), - )), - ], - ) - .unwrap(); + for (group, value) in values.into_iter().enumerate() { + let start = group as i32 * 1_000; + let s = arrow::compute::cast(&BinaryArray::from(vec![value; 500]), &s_type).unwrap(); + let a = Arc::new(Int32Array::from((start..start + 500).collect::>())); + let batch = RecordBatch::try_new(Arc::clone(&file_schema), vec![a, s]).unwrap(); writer.write(&batch).unwrap(); } writer.close().unwrap(); + PartitionedFile::from_path(filename).unwrap() + } - let partitioned_file = PartitionedFile::from_path(filename).unwrap(); - let session_ctx = Arc::new(SessionContext::new()); - let required_schema = Arc::new(Schema::new(vec![ + fn binary_as_string_required_schema() -> SchemaRef { + Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, false), Field::new("s", DataType::Utf8, true), - ])); - let scan = |filters: Option>>| { - init_datasource_exec( - Arc::clone(&required_schema), - Some(Arc::clone(&required_schema)), - None, - ObjectStoreUrl::local_filesystem(), - ObjectStoreBackend::Local, - vec![vec![partitioned_file.clone()]], - Some(vec![0, 1]), - filters, - None, - "UTC", - true, - false, - false, - false, - true, - &session_ctx, - false, - false, - false, - false, - "", - "", - ) - .unwrap() - }; + ])) + } - let full_scan = scan(None); - drain_scan(&full_scan, &session_ctx).await; + /// A session whose scans evaluate pushed filters on every row when `row_filter` is set, and + /// otherwise use them only to prune row groups. + fn binary_as_string_session(row_filter: bool) -> Arc { + let mut config = SessionConfig::new(); + config.options_mut().execution.parquet.pushdown_filters = row_filter; + Arc::new(SessionContext::new_with_config(config)) + } - let a_below_500: Arc = Arc::new(BinaryExpr::new( - Arc::new(Column::new("a", 0)), - Operator::Lt, - Arc::new(Literal::new(ScalarValue::Int32(Some(500)))), - )); - let s_equals: Arc = Arc::new(BinaryExpr::new( - Arc::new(Column::new("s", 1)), - Operator::Eq, - Arc::new(Literal::new(ScalarValue::Utf8(Some("x".to_string())))), - )); - let filtered_scan = scan(Some(vec![a_below_500, s_equals])); - let batches: Vec = filtered_scan + /// A scan of `file` with `binaryAsString` on, reading `s` as a string. + fn binary_as_string_scan( + file: &PartitionedFile, + filters: Vec>, + session_ctx: &Arc, + ) -> Arc { + let required_schema = binary_as_string_required_schema(); + init_datasource_exec( + Arc::clone(&required_schema), + Some(required_schema), + None, + ObjectStoreUrl::local_filesystem(), + ObjectStoreBackend::Local, + vec![vec![file.clone()]], + Some(vec![0, 1]), + Some(filters), + None, + "UTC", + true, + false, + false, + false, + true, + session_ctx, + false, + false, + false, + false, + "", + "", + ) + .unwrap() + } + + /// Counts the decoded values of `s` that `scan` returns. The scan has no residual filter, so + /// it returns every row of each row group its filters keep, or with row filters only the rows + /// they match. + async fn decoded_counts( + scan: &Arc, + session_ctx: &Arc, + ) -> DecodedCounts { + let batches: Vec = scan .execute(0, session_ctx.task_ctx()) .unwrap() .map(|batch| batch.unwrap()) .collect() .await; - - assert_eq!( - batches.iter().map(RecordBatch::num_rows).sum::(), - 500 - ); + let mut counts = BTreeMap::new(); for batch in &batches { assert_eq!(batch.schema().field(1).data_type(), &DataType::Utf8); - assert!(batch - .column(1) - .as_string::() - .iter() - .all(|value| value == Some("\u{fffd}"))); + for value in batch.column(1).as_string::() { + *counts.entry(value.map(str::to_string)).or_insert(0) += 1; + } } - let full_data = scan_metric(&full_scan, "scan_io_data_bytes"); - let filtered_data = scan_metric(&filtered_scan, "scan_io_data_bytes"); - assert!( - filtered_data < full_data, - "the filter on `a` should prune a row group: filtered={filtered_data}, full={full_data}" + counts + } + + async fn scan_binary_as_string( + file: &PartitionedFile, + filters: Vec>, + row_filter: bool, + ) -> DecodedCounts { + let session_ctx = binary_as_string_session(row_filter); + decoded_counts( + &binary_as_string_scan(file, filters, &session_ctx), + &session_ctx, + ) + .await + } + + fn compare_s(op: Operator, value: &str) -> Arc { + let literal = Literal::new(ScalarValue::Utf8(Some(value.to_string()))); + Arc::new(BinaryExpr::new( + Arc::new(Column::new("s", 1)), + op, + Arc::new(literal), + )) + } + + fn s_in(values: &[&str]) -> Arc { + let values = values + .iter() + .map(|value| { + let literal = Literal::new(ScalarValue::Utf8(Some(value.to_string()))); + Arc::new(literal) as Arc + }) + .collect(); + let schema = binary_as_string_required_schema(); + in_list(Arc::new(Column::new("s", 1)), values, &false, &schema).unwrap() + } + + fn groups(values: &[Option<&str>]) -> DecodedCounts { + values + .iter() + .map(|value| (value.map(str::to_string), 500)) + .collect() + } + + /// An unannotated BINARY column read as a string prunes on `=`, `IN` and `IS [NOT] NULL` + /// compared as raw bytes, while filters that could disagree with the decoded strings stay + /// above the scan. + #[tokio::test] + async fn binary_as_string_pushes_raw_byte_filters_on_unannotated_binary() { + let file = write_binary_as_string_fixture( + DataType::Binary, + [Some(b"one".as_slice()), Some(&[0xff]), None], + ); + let s = || Arc::new(Column::new("s", 1)) as Arc; + let a_below_1000: Arc = Arc::new(BinaryExpr::new( + Arc::new(Column::new("a", 0)), + Operator::Lt, + Arc::new(Literal::new(ScalarValue::Int32(Some(1_000)))), + )); + let every_group = groups(&[Some("one"), Some("\u{fffd}"), None]); + let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ + ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), + ("IN", s_in(&["one", "zzz"]), groups(&[Some("one")])), + ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), + ( + "IS NOT NULL", + Arc::new(IsNotNullExpr::new(s())), + groups(&[Some("one"), Some("\u{fffd}")]), + ), + // Pushed, this would prune the null group. It isn't, because a range on the decoded + // strings can disagree with the same range on the bytes. + ("range", compare_s(Operator::Gt, "a"), every_group.clone()), + // The invalid group decodes to U+FFFD, so comparing the literal's bytes would prune a + // group that matches. + ( + "U+FFFD literal", + compare_s(Operator::Eq, "\u{fffd}"), + every_group, + ), + // A filter on another column still prunes, and `AND` keeps the side that is pushed. + ( + "AND", + Arc::new(BinaryExpr::new( + a_below_1000, + Operator::And, + compare_s(Operator::Gt, "a"), + )), + groups(&[Some("one")]), + ), + ]; + for (name, filter, expected) in cases { + for row_filter in [false, true] { + assert_eq!( + scan_binary_as_string(&file, vec![Arc::clone(&filter)], row_filter).await, + expected, + "{name}, row filter {row_filter}" + ); + } + } + } + + /// A join or TopK dynamic filter is pushed onto the scan's output, which holds the decoded + /// strings, so the scan maps it through its decoding projection and it compares decoded + /// values rather than bytes. `"a\x80"` sorts below `"aé"` as bytes, but it decodes to + /// `"a\u{FFFD}"`, which sorts above it. + #[tokio::test] + async fn binary_as_string_output_filter_compares_decoded_strings() { + let file = write_binary_as_string_fixture( + DataType::Binary, + [Some(b"a\x80".as_slice()), Some(b"a"), None], + ); + let session_ctx = binary_as_string_session(true); + let scan = binary_as_string_scan(&file, vec![], &session_ctx); + let config = session_ctx.copied_config(); + let pushed = scan + .data_source() + .try_pushdown_filters(vec![compare_s(Operator::Lt, "aé")], config.options()) + .unwrap() + .updated_node + .expect("the scan should accept the filter"); + let scan = Arc::new(scan.as_ref().clone().with_data_source(pushed)); + assert_eq!( + decoded_counts(&scan, &session_ctx).await, + groups(&[Some("a")]) + ); + } + + /// A column the file annotates as a string is read as Binary too, and the adapter's cast back + /// would hide it from pruning. The adapter compares the column directly instead. + #[tokio::test] + async fn binary_as_string_prunes_annotated_strings() { + let file = write_binary_as_string_fixture( + DataType::Utf8, + [Some(b"one".as_slice()), Some(b"two"), None], ); + let s = || Arc::new(Column::new("s", 1)) as Arc; + let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ + ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), + ("IN", s_in(&["two"]), groups(&[Some("two")])), + ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), + ( + "IS NOT NULL", + Arc::new(IsNotNullExpr::new(s())), + groups(&[Some("one"), Some("two")]), + ), + ]; + for (name, filter, expected) in cases { + for row_filter in [false, true] { + assert_eq!( + scan_binary_as_string(&file, vec![Arc::clone(&filter)], row_filter).await, + expected, + "{name}, row filter {row_filter}" + ); + } + } } fn write_scan_io_fixture() -> (String, SchemaRef) { diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index b3f93e18762..bf55c6327e4 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -32,7 +32,10 @@ use arrow::datatypes::{DataType, Field, FieldRef, Schema, SchemaRef, TimeUnit}; use arrow::record_batch::RecordBatch; use datafusion::common::tree_node::{Transformed, TransformedResult, TreeNode, TreeNodeRecursion}; use datafusion::common::{DataFusionError, Result as DataFusionResult}; -use datafusion::physical_expr::expressions::Column; +use datafusion::logical_expr::Operator; +use datafusion::physical_expr::expressions::{ + in_list, BinaryExpr, CastExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, Literal, +}; use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_plan::ColumnarValue; use datafusion::scalar::ScalarValue; @@ -910,6 +913,24 @@ fn check_decoded_field_names(data_type: &DataType) -> DataFusionResult<()> { Ok(()) } +/// A Binary literal `expr` as a literal of `data_type` holding the same bytes, or `None` when it +/// isn't a non-null Binary literal or its bytes aren't valid UTF-8 for a string type. +fn binary_literal_as( + expr: &Arc, + data_type: &DataType, +) -> Option> { + let ScalarValue::Binary(Some(bytes)) = expr.downcast_ref::()?.value() else { + return None; + }; + let value = match data_type { + DataType::Utf8 | DataType::LargeUtf8 | DataType::Utf8View => { + ScalarValue::Utf8(Some(String::from_utf8(bytes.clone()).ok()?)) + } + _ => ScalarValue::Binary(Some(bytes.clone())), + }; + Some(Arc::new(Literal::new(value.cast_to(data_type).ok()?))) +} + /// Check an expression before it can decode its entire physical subtree. fn checked_decoded_expr( physical_type: &DataType, @@ -1185,6 +1206,8 @@ impl PhysicalExprAdapter for SparkPhysicalExprAdapter { // - CometCastColumnExpr (for Struct/List/Map, uses spark_parquet_convert) // - Spark Cast (for simple scalar types) rewritten + .transform(|e| self.unwrap_cast_to_binary_comparison(e)) + .data()? .transform(|e| self.replace_with_spark_cast(e)) .data()? } @@ -1409,6 +1432,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( + &self, + expr: Arc, + ) -> DataFusionResult>> { + if let Some(binary) = expr.downcast_ref::() { + if binary.op() == &Operator::Eq { + let (left, right) = (binary.left(), binary.right()); + if let Some((column, data_type)) = self.column_cast_to_binary(left) { + if let Some(literal) = binary_literal_as(right, &data_type) { + let eq = BinaryExpr::new(column, Operator::Eq, literal); + return Ok(Transformed::yes(Arc::new(eq))); + } + } + if let Some((column, data_type)) = self.column_cast_to_binary(right) { + if let Some(literal) = binary_literal_as(left, &data_type) { + let eq = BinaryExpr::new(literal, Operator::Eq, column); + return Ok(Transformed::yes(Arc::new(eq))); + } + } + } + } else if let Some(list) = expr.downcast_ref::() { + if let Some((column, data_type)) = self.column_cast_to_binary(list.expr()) { + let values: Option> = list + .list() + .iter() + .map(|value| binary_literal_as(value, &data_type)) + .collect(); + if let Some(values) = values { + let negated = list.negated(); + let list = in_list(column, values, &negated, &self.physical_file_schema)?; + return Ok(Transformed::yes(list)); + } + } + } else if let Some(is_null) = expr.downcast_ref::() { + if let Some((column, _)) = self.column_cast_to_binary(is_null.arg()) { + return Ok(Transformed::yes(Arc::new(IsNullExpr::new(column)))); + } + } else if let Some(is_not_null) = expr.downcast_ref::() { + if let Some((column, _)) = self.column_cast_to_binary(is_not_null.arg()) { + return Ok(Transformed::yes(Arc::new(IsNotNullExpr::new(column)))); + } + } + Ok(Transformed::no(expr)) + } + + /// The column and its file type when `expr` casts a string or binary column to Binary, which + /// keeps every value's bytes. + fn column_cast_to_binary( + &self, + expr: &Arc, + ) -> Option<(Arc, DataType)> { + let cast = expr.downcast_ref::()?; + if cast.target_field().data_type() != &DataType::Binary { + return None; + } + let column = cast.expr(); + column.downcast_ref::()?; + let data_type = column.data_type(&self.physical_file_schema).ok()?; + matches!( + data_type, + DataType::Utf8 + | DataType::LargeUtf8 + | DataType::Utf8View + | DataType::LargeBinary + | DataType::BinaryView + ) + .then(|| (Arc::clone(column), data_type)) + } + /// Replace CastExpr (DataFusion's cast) with Spark's Cast expression. /// /// Only the casts that reconcile a file column with its table-schema type are Parquet read diff --git a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala index f6c1ad918d4..5d1e6920273 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -43,6 +43,7 @@ import org.apache.spark.sql.catalyst.optimizer.{ConvertToLocalRelation, Optimize import org.apache.spark.sql.catalyst.util.DateTimeUtils import org.apache.spark.sql.comet.{CometNativeScanExec, CometScanExec} import org.apache.spark.sql.comet.util.Utils +import org.apache.spark.sql.execution.SparkPlan import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper import org.apache.spark.sql.execution.datasources.SchemaColumnConvertNotSupportedException import org.apache.spark.sql.execution.datasources.parquet.ParquetUtils @@ -358,6 +359,81 @@ abstract class ParquetReadSuite extends CometTestBase { } } + // With binaryAsString, the native scan compares `=`, `IN` and `IS [NOT] NULL` on the raw bytes + // and keeps other string filters above the scan. Spark compares raw bytes too, so these queries + // match it. A query that tells a malformed value apart from U+FFFD doesn't, as the + // compatibility guide describes. + test("spark.sql.parquet.binaryAsString filters, joins and top-k") { + withTempDir { dir => + val schema = MessageTypeParser.parseMessageType(""" + |message root { + | optional binary value; + | optional int32 id; + |} + |""".stripMargin) + // Two partition directories, each holding a valid value, the malformed byte 0xff and a null. + val partitioned = new File(dir, "partitioned").getCanonicalPath + Seq(1 -> "one", 2 -> "two").foreach { case (p, valid) => + val writer = createParquetWriter(schema, new Path(s"$partitioned/p=$p/part-r-0.parquet")) + Seq( + Some(Binary.fromString(valid)), + Some(Binary.fromConstantByteArray(Array(0xff.toByte))), + None).zipWithIndex.foreach { case (value, id) => + val record = new SimpleGroup(schema) + value.foreach(record.add(0, _)) + record.add(1, p * 10 + id) + writer.write(record) + } + writer.close() + } + val annotated = new File(dir, "annotated").getCanonicalPath + Seq(Some("one"), Some("two"), None).toDF("value").write.parquet(annotated) + + def assertNativeScan(plan: SparkPlan): Unit = + assert( + collect(plan) { case _: CometNativeScanExec => true }.nonEmpty, + s"Expected a native scan:\n${plan.treeString}") + + Seq("false", "true").foreach { rowFilter => + withSQLConf( + SQLConf.PARQUET_BINARY_AS_STRING.key -> "true", + CometConf.COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED.key -> rowFilter) { + spark.read.parquet(partitioned).createOrReplaceTempView("t") + spark.read.parquet(annotated).createOrReplaceTempView("annotated") + Seq( + "SELECT * FROM t WHERE value = 'one'", + "SELECT * FROM t WHERE value IN ('one', 'two')", + "SELECT * FROM t WHERE value IS NULL", + "SELECT * FROM t WHERE value IS NOT NULL", + "SELECT * FROM t WHERE value > 'o'", + "SELECT * FROM t WHERE value = 'one' OR id = 21", + "SELECT * FROM annotated WHERE value = 'one'", + "SELECT * FROM annotated WHERE value IS NULL").foreach { query => + val (_, plan) = checkSparkAnswerAndOperator(sql(query)) + assertNativeScan(plan) + } + Seq( + "SELECT a.id, b.id FROM t a JOIN t b ON a.value = b.value", + "SELECT value, id FROM t ORDER BY value, id LIMIT 3").foreach { query => + val (_, plan) = checkSparkAnswer(sql(query)) + assertNativeScan(plan) + } + if (rowFilter == "true") { + // Spark also pushes `isnotnull(value)`, which on its own would let the other non-null + // values through. Only the pushed `value = 'one'` leaves a single row. + Seq("t", "annotated").foreach { table => + val (_, plan) = checkSparkAnswer(sql(s"SELECT * FROM $table WHERE value = 'one'")) + val matched = collect(plan) { case scan: CometNativeScanExec => + scan.metrics("pushdown_rows_matched").value + }.sum + assert(matched == 1, s"Expected the $table scan to filter on `value = 'one'`") + } + } + } + } + } + } + test("string") { val data = (1 to 4).map(i => Tuple1(i.toString)) // Property spark.sql.parquet.binaryAsString shouldn't affect Parquet files written by Spark SQL From 7f8410893ba4f179f7d87375d3dcdb502a3763d6 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 21 Oct 2026 01:02:39 +0800 Subject: [PATCH 04/10] test: benchmark annotated string scans with binaryAsString off and on With spark.sql.parquet.binaryAsString enabled, the native scan reads Spark-written strings as Binary too and decodes them back. Add a full scan and a selective equality filter over sorted strings in small row groups, each with the setting off and on. Run it alone with --binary-as-string. Co-Authored-By: Claude Opus 5.5 --- .../sql/benchmark/CometReadBenchmark.scala | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala index caf41680505..a6f7bbe02d2 100644 --- a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala +++ b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala @@ -35,6 +35,7 @@ import org.apache.spark.benchmark.Benchmark import org.apache.spark.sql.{DataFrame, SparkSession} import org.apache.spark.sql.execution.datasources.parquet.VectorizedParquetRecordReader import org.apache.spark.sql.execution.metric.SQLMetrics +import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ import org.apache.spark.sql.vectorized.ColumnVector @@ -422,11 +423,51 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { } } + /** + * Spark-written strings carry the UTF8 annotation. With `spark.sql.parquet.binaryAsString`, the + * native scan still reads them as binary and decodes them, and only `=`, `IN` and `IS [NOT] + * NULL` on them reach the reader. This compares a full scan and a selective filter with the + * setting off and on. + */ + def binaryAsStringScanBenchmark(values: Int): Unit = { + withTempPath { dir => + withTempTable("parquetV1Table") { + // Sorted values in small row groups, so `c1 = '500'` can skip most of them. + withSQLConf("parquet.block.size" -> (1024 * 1024).toString) { + prepareTable( + dir, + spark.sql(s"SELECT CAST(value % 1000 AS STRING) AS c1 FROM $tbl ORDER BY c1")) + } + Seq( + "Annotated String Scan" -> "SELECT sum(length(c1)) FROM parquetV1Table", + "Annotated String Filter" -> + "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 = '500'").foreach { + case (name, query) => + val benchmark = new Benchmark(name, values.toLong, output = output) + Seq("false", "true").foreach { binaryAsString => + addParquetScanCases( + benchmark, + query, + s"binaryAsString=$binaryAsString", + Map(SQLConf.PARQUET_BINARY_AS_STRING.key -> binaryAsString)) + } + benchmark.run() + } + } + } + } + override def runCometBenchmark(mainArgs: Array[String]): Unit = { if (mainArgs.contains("--scan-metric-overhead")) { scanMetricAccumulatorBenchmark(mainArgs.contains("--reverse-cases")) return } + if (mainArgs.contains("--binary-as-string")) { + runBenchmarkWithTable("binaryAsString Scan", 1024 * 1024 * 10) { v => + binaryAsStringScanBenchmark(v) + } + return + } runBenchmarkWithTable("Parquet Reader", 1024 * 1024 * 15) { v => Seq( BooleanType, @@ -499,6 +540,10 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { sortedLgStrFilterScanBenchmark(v, fractionOfZeros) } } + + runBenchmarkWithTable("binaryAsString Scan", 1024 * 1024 * 10) { v => + binaryAsStringScanBenchmark(v) + } } } From ead8d2aadd41f2ded147289bd9114908f1ae0f1e Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 23 Oct 2026 00:15:36 +0800 Subject: [PATCH 05/10] test: cover NOT IN and non-UTF-8 literals in binaryAsString filters The scan tests now check that NOT IN and an IN list holding U+FFFD stay above the scan, and ParquetReadSuite compares NOT IN with Spark. A schema adapter test checks that NOT IN on an annotated column is compared directly, and that a null or invalid UTF-8 Binary literal leaves the filter unchanged. The compatibility guide no longer says Parquet text is always valid UTF-8, and the configuration audit marks binaryAsString as Partial, a status its legend defines. Co-Authored-By: Claude Opus 5.5 --- .../spark_configs_support.md | 10 ++- .../user-guide/latest/compatibility/index.md | 3 +- native/core/src/parquet/parquet_exec.rs | 21 ++++- native/core/src/parquet/schema_adapter.rs | 85 +++++++++++++++++++ .../comet/parquet/ParquetReadSuite.scala | 1 + 5 files changed, 111 insertions(+), 9 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 39d69c3d067..396135b14df 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -52,7 +52,7 @@ The status column uses these values: - Date: 2026-07-18 - `spark.sql.parquet.binaryAsString` - Default: `false` - - Status: Native (see notes) + - Status: Partial (see notes) - Affected operator: native Parquet scan - Date: 2026-10-21 @@ -194,11 +194,13 @@ and verifies the inferred schema under both configuration values, Spark answer p `CometNativeScanExec`, including a row containing invalid UTF-8 bytes and a projection that reads only a non-string column. A second test reads two partition directories holding a valid value, `0xff` and a null, and a Spark-written file, with row-filter pushdown off and on. It compares `=`, -`IN`, `IS [NOT] NULL`, a range, an `OR`, a self-join on the string column and an +`IN`, `NOT IN`, `IS [NOT] NULL`, a range, an `OR`, a self-join on the string column and an `ORDER BY ... LIMIT` against Spark, and checks that `value = 'one'` reaches the row filter. Native tests in `parquet_exec.rs` check which row groups each filter prunes, for an unannotated and an -annotated column, with and without row filters. They include a range and a `U+FFFD` literal, -neither of which may prune. +annotated column, with and without row filters. They include `NOT IN`, a range and `U+FFFD` +literals, none of which may prune. A native test in `schema_adapter.rs` checks that the adapter +compares an annotated column directly for `NOT IN` as for `IN`, and leaves a filter with a null or +invalid UTF-8 literal unchanged. **Known divergences.** Decoding is not byte-preserving, so the differences described in [Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any diff --git a/docs/source/user-guide/latest/compatibility/index.md b/docs/source/user-guide/latest/compatibility/index.md index dca663d9228..406e6ccb250 100644 --- a/docs/source/user-guide/latest/compatibility/index.md +++ b/docs/source/user-guide/latest/compatibility/index.md @@ -106,7 +106,8 @@ a result: - **Parquet BINARY columns read as strings.** A BINARY column without the UTF8 annotation can hold bytes that are not valid UTF-8 when it is read as `string`, through the read schema or `spark.sql.parquet.binaryAsString=true`. Comet decodes such a value in a struct field, list element - or map value. Top-level values and map keys are not checked; see + or map value, and a top-level value when `spark.sql.parquet.binaryAsString=true`. A top-level + value read through the read schema, and a map key, are not checked; see [Parquet](../parquet.md#reads-that-differ-from-spark). - **Text files.** Spark's Text reader keeps the bytes of each line as they are, so a Text file in an encoding other than UTF-8, such as Latin-1, holds strings that are not valid UTF-8. Comet decodes diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index e8f9e5a7c00..6ce23407d4c 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -898,7 +898,7 @@ mod tests { )) } - fn s_in(values: &[&str]) -> Arc { + fn s_in(values: &[&str], negated: bool) -> Arc { let values = values .iter() .map(|value| { @@ -907,7 +907,7 @@ mod tests { }) .collect(); let schema = binary_as_string_required_schema(); - in_list(Arc::new(Column::new("s", 1)), values, &false, &schema).unwrap() + in_list(Arc::new(Column::new("s", 1)), values, &negated, &schema).unwrap() } fn groups(values: &[Option<&str>]) -> DecodedCounts { @@ -935,7 +935,7 @@ mod tests { let every_group = groups(&[Some("one"), Some("\u{fffd}"), None]); let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), - ("IN", s_in(&["one", "zzz"]), groups(&[Some("one")])), + ("IN", s_in(&["one", "zzz"], false), groups(&[Some("one")])), ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), ( "IS NOT NULL", @@ -945,11 +945,19 @@ mod tests { // Pushed, this would prune the null group. It isn't, because a range on the decoded // strings can disagree with the same range on the bytes. ("range", compare_s(Operator::Gt, "a"), every_group.clone()), + // The scan pushes only `=`, `IN` and `IS [NOT] NULL`, so `NOT IN` stays above it. + ("NOT IN", s_in(&["one"], true), every_group.clone()), // The invalid group decodes to U+FFFD, so comparing the literal's bytes would prune a // group that matches. ( "U+FFFD literal", compare_s(Operator::Eq, "\u{fffd}"), + every_group.clone(), + ), + // Nor is an `IN` list holding one: pushing its other values would prune that group too. + ( + "IN with a U+FFFD literal", + s_in(&["one", "\u{fffd}"], false), every_group, ), // A filter on another column still prunes, and `AND` keeps the side that is pushed. @@ -1011,7 +1019,12 @@ mod tests { let s = || Arc::new(Column::new("s", 1)) as Arc; let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), - ("IN", s_in(&["two"]), groups(&[Some("two")])), + ("IN", s_in(&["two"], false), groups(&[Some("two")])), + ( + "NOT IN", + s_in(&["one"], true), + groups(&[Some("one"), Some("two"), None]), + ), ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), ( "IS NOT NULL", diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index bf55c6327e4..f4bf12da058 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -5600,4 +5600,89 @@ pub(crate) mod test { "expected duplicate field id error, got: {msg}" ); } + + /// A `binaryAsString` scan reads a column the file annotates as a string as Binary, so the + /// default adapter casts the column to Binary in a pushed filter. The adapter compares the + /// column with string literals instead, for `NOT IN` as for `IN`. It leaves the filter + /// unchanged when a literal is null or isn't valid UTF-8, since no string literal stands for it. + #[test] + fn binary_filter_on_annotated_string_compares_column_only_for_utf8_literals() { + use datafusion::physical_expr::expressions::{in_list, InListExpr}; + + fn s() -> Arc { + Arc::new(Column::new("s", 0)) + } + fn binary(bytes: Option<&[u8]>) -> Arc { + Arc::new(Literal::new(ScalarValue::Binary(bytes.map(<[u8]>::to_vec)))) + } + + let logical = Arc::new(Schema::new(vec![Field::new("s", DataType::Binary, true)])); + let physical = Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, true)])); + let adapter = SparkPhysicalExprAdapterFactory::new( + SparkParquetOptions::new(EvalMode::Legacy, "UTC"), + None, + ) + .create(Arc::clone(&logical), Arc::clone(&physical)) + .unwrap(); + let batch = RecordBatch::try_new( + Arc::clone(&physical), + vec![Arc::new(StringArray::from(vec![ + Some("one"), + Some("two"), + None, + ]))], + ) + .unwrap(); + let s_eq = |bytes: Option<&[u8]>| -> Arc { + Arc::new(BinaryExpr::new(s(), Operator::Eq, binary(bytes))) + }; + let s_in = |values: Vec>, negated: bool| { + let values = values.into_iter().map(binary).collect(); + in_list(s(), values, &negated, &logical).unwrap() + }; + let one = Some(b"one".as_slice()); + let invalid = Some(b"\xff".as_slice()); + + // Each case gives whether the rewritten filter compares the column itself, and its result. + let cases = [ + ( + "NOT IN", + s_in(vec![one], true), + true, + [Some(false), Some(true), None], + ), + ( + "invalid UTF-8", + s_eq(invalid), + false, + [Some(false), Some(false), None], + ), + ("null", s_eq(None), false, [None, None, None]), + ( + "IN with invalid UTF-8", + s_in(vec![one, invalid], false), + false, + [Some(true), Some(false), None], + ), + ]; + for (name, filter, compares_column, expected) in cases { + let rewritten = adapter.rewrite(filter).unwrap(); + let operand = match rewritten.downcast_ref::() { + Some(list) => list.expr(), + None => rewritten.downcast_ref::().unwrap().left(), + }; + assert_eq!( + operand.downcast_ref::().is_some(), + compares_column, + "{name}" + ); + let result = rewritten.evaluate(&batch).unwrap(); + let result = result.into_array(batch.num_rows()).unwrap(); + assert_eq!( + result.as_boolean().iter().collect::>(), + expected, + "{name}" + ); + } + } } diff --git a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala index 5d1e6920273..c9db74310dc 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -403,6 +403,7 @@ abstract class ParquetReadSuite extends CometTestBase { Seq( "SELECT * FROM t WHERE value = 'one'", "SELECT * FROM t WHERE value IN ('one', 'two')", + "SELECT * FROM t WHERE value NOT IN ('one', 'two')", "SELECT * FROM t WHERE value IS NULL", "SELECT * FROM t WHERE value IS NOT NULL", "SELECT * FROM t WHERE value > 'o'", From 186f1b47787abcf3f613083ba3a04a5a1095fe1c Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 4 Nov 2026 23:18:50 +0800 Subject: [PATCH 06/10] refactor: bind binaryAsString filter columns by index instead of folded name The planner binds data filters by position against the required columns followed by the partition columns, so a column's index says which field it reads. Check the required schema's field type at that index instead of folding column names into a set, and drop the fold_name and fold_names imports. A scan that projects no string column now keeps the projection-indices path, since it has nothing to read as Binary. Co-Authored-By: Claude Fable 5.1 --- native/core/src/parquet/parquet_exec.rs | 137 ++++++++++++++---------- 1 file changed, 83 insertions(+), 54 deletions(-) diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index 6ce23407d4c..fcb628341b0 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -19,7 +19,7 @@ use crate::execution::operators::ExecutionError; use crate::parquet::eager_page_index_reader_factory::{EagerPageIndexReaderFactory, ScanIoSource}; use crate::parquet::encryption_support::{CometEncryptionConfig, ENCRYPTION_FACTORY_ID}; use crate::parquet::file_error_context::ParquetErrorContext; -use crate::parquet::name_fold::{fold_name, fold_names, fold_schema_names}; +use crate::parquet::name_fold::fold_schema_names; use crate::parquet::parquet_support::{ object_store_authority, ObjectStoreBackend, SparkParquetOptions, }; @@ -47,7 +47,7 @@ use datafusion_comet_spark_expr::jvm_udf::JvmScalarUdfExpr; use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions}; use datafusion_datasource::TableSchema; use parquet::variant::VariantType; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; #[cfg(test)] @@ -100,18 +100,19 @@ fn is_binary_as_string_read(data_type: &DataType) -> bool { data_type == &DataType::Utf8 } -/// Whether `expr` references a column whose folded name is in `folded_names`. -fn references_any_column( - expr: &Arc, - folded_names: &HashSet, - case_sensitive: bool, -) -> Result { - for column in collect_columns(expr) { - if folded_names.contains(&fold_name(column.name(), case_sensitive)?) { - return Ok(true); - } - } - Ok(false) +/// Whether `column`, bound against the required columns followed by the partition columns the +/// way the planner binds data filters, is a string column that `binary_as_string_read_schema` +/// reads as Binary. +fn reads_binary(column: &Column, required_schema: &Schema) -> bool { + column.index() < required_schema.fields().len() + && is_binary_as_string_read(required_schema.field(column.index()).data_type()) +} + +/// Whether `expr` references a column `binary_as_string_read_schema` reads as Binary. +fn references_binary_read_column(expr: &Arc, required_schema: &Schema) -> bool { + collect_columns(expr) + .iter() + .any(|column| reads_binary(column, required_schema)) } /// Rewrites a data filter for a scan that reads string columns as Binary (see @@ -128,28 +129,18 @@ fn references_any_column( /// keep every row the original keeps. That's why an `AND` keeps whichever side can be pushed. fn binary_as_string_pushdown_filter( filter: &Arc, - binary_read_columns: &HashSet, - case_sensitive: bool, + required_schema: &Schema, filter_read_schema: &Schema, ) -> Result>, ExecutionError> { - if !references_any_column(filter, binary_read_columns, case_sensitive)? { + if !references_binary_read_column(filter, required_schema) { return Ok(Some(Arc::clone(filter))); } - let reads_binary = |expr: &Arc| -> Result { - match expr.downcast_ref::() { - Some(column) => { - Ok(binary_read_columns.contains(&fold_name(column.name(), case_sensitive)?)) - } - None => Ok(false), - } + let reads_binary = |expr: &Arc| { + expr.downcast_ref::() + .is_some_and(|column| reads_binary(column, required_schema)) }; let pushdown = |expr: &Arc| { - binary_as_string_pushdown_filter( - expr, - binary_read_columns, - case_sensitive, - filter_read_schema, - ) + binary_as_string_pushdown_filter(expr, required_schema, filter_read_schema) }; if let Some(binary) = filter.downcast_ref::() { @@ -172,13 +163,13 @@ fn binary_as_string_pushdown_filter( }); } Operator::Eq => { - if reads_binary(left)? { + if reads_binary(left) { if let Some(literal) = binary_literal(right) { let eq = BinaryExpr::new(Arc::clone(left), Operator::Eq, literal); return Ok(Some(Arc::new(eq))); } } - if reads_binary(right)? { + if reads_binary(right) { if let Some(literal) = binary_literal(left) { let eq = BinaryExpr::new(literal, Operator::Eq, Arc::clone(right)); return Ok(Some(Arc::new(eq))); @@ -188,18 +179,18 @@ fn binary_as_string_pushdown_filter( _ => {} } } else if let Some(list) = filter.downcast_ref::() { - if !list.negated() && reads_binary(list.expr())? { + if !list.negated() && reads_binary(list.expr()) { if let Some(values) = list.list().iter().map(binary_literal).collect() { let expr = Arc::clone(list.expr()); return Ok(Some(in_list(expr, values, &false, filter_read_schema)?)); } } } else if let Some(is_null) = filter.downcast_ref::() { - if reads_binary(is_null.arg())? { + if reads_binary(is_null.arg()) { return Ok(Some(Arc::clone(filter))); } } else if let Some(is_not_null) = filter.downcast_ref::() { - if reads_binary(is_not_null.arg())? { + if reads_binary(is_not_null.arg()) { return Ok(Some(Arc::clone(filter))); } } @@ -388,6 +379,13 @@ pub(crate) fn init_datasource_exec( let output_table_schema = TableSchema::builder(Arc::clone(&base_schema)) .with_table_partition_cols(partition_fields.clone()) .build(); + // A scan that projects no string column has nothing to read as Binary, so it keeps the + // projection-indices path below. + let binary_as_string = binary_as_string + && required_schema + .fields() + .iter() + .any(|field| is_binary_as_string_read(field.data_type())); let read_base_schema = if binary_as_string { binary_as_string_read_schema(&base_schema) } else { @@ -397,20 +395,6 @@ pub(crate) fn init_datasource_exec( .with_table_partition_cols(partition_fields.clone()) .build(); - // Folded names of the columns read as binary. Filters that reference one are rewritten for - // the binary read below, or kept out of the scan. - let binary_read_columns: HashSet = if binary_as_string { - let names: Vec<&str> = base_schema - .fields() - .iter() - .filter(|field| is_binary_as_string_read(field.data_type())) - .map(|field| field.name().as_str()) - .collect(); - fold_names(&names, case_sensitive)?.into_iter().collect() - } else { - HashSet::new() - }; - let binary_projection = binary_as_string.then(|| { binary_as_string_projection( table_schema.table_schema(), @@ -520,9 +504,9 @@ pub(crate) fn init_datasource_exec( // reads the string columns as Binary. Rewrite the filters on those columns that can compare // raw bytes, and keep the rest out of the scan. Filters on other columns are pushed as is. let data_filters = match data_filters { - Some(filters) if !binary_read_columns.is_empty() => { + Some(filters) if binary_as_string => { // The planner binds data filters against the required columns followed by the - // partition columns. + // partition columns, so a column's index says which field it reads. let filter_read_schema = Schema::new( required_schema .fields() @@ -535,8 +519,7 @@ pub(crate) fn init_datasource_exec( for filter in filters { if let Some(filter) = binary_as_string_pushdown_filter( &filter, - &binary_read_columns, - case_sensitive, + &required_schema, &filter_read_schema, )? { pushable.push(filter); @@ -728,7 +711,7 @@ mod tests { use datafusion::logical_expr::Operator; use datafusion::physical_expr::expressions::{BinaryExpr, Literal}; use datafusion::physical_plan::metrics::ExecutionPlanMetricsSet; - use datafusion::physical_plan::ExecutionPlan; + use datafusion::physical_plan::{displayable, ExecutionPlan}; use datafusion::prelude::SessionConfig; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; use futures::StreamExt; @@ -917,6 +900,52 @@ mod tests { .collect() } + /// A `binaryAsString` scan that projects no string column has nothing to read as Binary, so + /// it builds the same plan as with the setting off, with the projection as indices. + #[test] + fn binary_as_string_leaves_scan_without_string_columns_unchanged() { + let file = write_binary_as_string_fixture(DataType::Binary, [Some(b"one"), None, None]); + let required_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)])); + let session_ctx = binary_as_string_session(false); + let plan = |binary_as_string: bool| { + let filter: Arc = Arc::new(BinaryExpr::new( + Arc::new(Column::new("a", 0)), + Operator::Eq, + Arc::new(Literal::new(ScalarValue::Int32(Some(1)))), + )); + let scan = init_datasource_exec( + Arc::clone(&required_schema), + Some(binary_as_string_required_schema()), + None, + ObjectStoreUrl::local_filesystem(), + ObjectStoreBackend::Local, + vec![vec![file.clone()]], + Some(vec![0]), + Some(vec![filter]), + None, + "UTC", + true, + false, + false, + false, + binary_as_string, + &session_ctx, + false, + false, + false, + false, + "", + "", + ) + .unwrap(); + let plan = displayable(scan.as_ref()).indent(true).to_string(); + plan + }; + let plan_off = plan(false); + assert!(plan_off.contains("projection=[a]"), "{plan_off}"); + assert_eq!(plan(true), plan_off); + } + /// An unannotated BINARY column read as a string prunes on `=`, `IN` and `IS [NOT] NULL` /// compared as raw bytes, while filters that could disagree with the decoded strings stay /// above the scan. From 00fbaf6fd9315bc7bd3c317fde7d50aba22a76af Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 4 Nov 2026 23:21:41 +0800 Subject: [PATCH 07/10] docs: record the Spark versions checked for the binaryAsString audit The schema converter's binaryAsString inference and the BINARY cases of ParquetFilters are the same in 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. Co-Authored-By: Claude Fable 5.1 --- docs/source/contributor-guide/spark_configs_support.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 396135b14df..944813c2a57 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -54,6 +54,7 @@ The status column uses these values: - Default: `false` - Status: Partial (see notes) - Affected operator: native Parquet scan + - Spark versions checked: 3.4.3, 3.5.9, 4.0.4, 4.1.3, 4.2.0 - Date: 2026-10-21 ## Audit Notes @@ -161,7 +162,9 @@ the Comet pipeline. Tests for expressions with known fallback paths continue to **Source.** When enabled, Spark infers unannotated Parquet `BINARY` columns as `StringType`. Parquet files written by Spark carry Spark schema metadata, so this -configuration does not change the types inferred for those files. +configuration does not change the types inferred for those files. The inference in +`ParquetToSparkSchemaConverter` and the `BINARY` cases of `ParquetFilters` are the same in every +Spark version listed above. **Comet status.** Comet honors the configuration in the native Parquet scan. When it is enabled, the scan reads every top-level string column as Arrow `Binary` and decodes it to valid From 1fe3c6731b2c2da5ed4649df690980e0c1c6d2f5 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Sun, 8 Nov 2026 00:46:05 +0900 Subject: [PATCH 08/10] fix: preserve binaryAsString type checks and annotated string pruning --- .../spark_configs_support.md | 77 +++--- docs/source/user-guide/latest/parquet.md | 17 +- .../eager_page_index_reader_factory.rs | 146 ++++++++++- native/core/src/parquet/parquet_exec.rs | 243 ++++++++++++++---- native/core/src/parquet/parquet_support.rs | 5 + native/core/src/parquet/schema_adapter.rs | 138 +++++++++- .../comet/parquet/ParquetReadSuite.scala | 36 ++- .../sql/benchmark/CometReadBenchmark.scala | 11 +- 8 files changed, 579 insertions(+), 94 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index 944813c2a57..ff8730b93bf 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -166,44 +166,49 @@ configuration does not change the types inferred for those files. The inference `ParquetToSparkSchemaConverter` and the `BINARY` cases of `ParquetFilters` are the same in every Spark version listed above. -**Comet status.** Comet honors the configuration in the native Parquet scan. When it is -enabled, the scan reads every top-level string column as Arrow `Binary` and decodes it to valid -UTF-8 at the scan boundary using Spark-compatible replacement semantics. Spark's schema doesn't -say which file columns carry the UTF8 annotation, so annotated columns take the same path. -arrow-rs validates UTF-8 only for Parquet columns annotated as strings, so decoding unannotated -columns straight into Arrow `Utf8` would put invalid UTF-8 into a string array, which is undefined -behavior. The default `false` value continues to return unannotated `BINARY` columns as +**Comet status.** Comet honors the configuration in the native Parquet scan. When enabled, +its logical read schema uses Arrow `Binary` for top-level strings and a projection decodes those +bytes to valid UTF-8. For unencrypted files, the reader derives a private field marker from each +column's actual Parquet UTF8 annotation. The expression adapter uses that marker to remove the +synthetic binary-to-string round trip for validated string columns. An Arrow `Utf8` type alone is +not enough: JSON and an `ARROW:schema` string hint over unannotated BINARY do not imply validation. +Encrypted scans retain the decode because rebuilding the footer would lose private decryption +state in the pinned Parquet API. The default `false` continues to infer unannotated BINARY as `BinaryType`. -**Filter pushdown.** While the configuration is enabled, the scan rewrites `=`, `IN` and -`IS [NOT] NULL` on a top-level string column to compare raw bytes, so `value = 'one'` becomes -`value = X'6F6E65'`. A valid value decodes to itself, and a malformed one to a string containing -`U+FFFD`, so for a literal without `U+FFFD` the rewritten filter keeps exactly the rows the -original keeps. For a file that annotates the column as a string, the expression adapter compares -the column with the string literal directly, so the file's statistics still prune. Other filters -on these columns stay above the scan, because decoding can reorder values: `"a\x80"` sorts below -`"aé"` as bytes, but `"a\uFFFD"` sorts above it. That covers ranges, `LIKE`, function calls and -literals containing `U+FFFD`. Spark's `Filter` above the scan re-evaluates every data filter, so -results don't depend on what is pushed. Join and TopK dynamic filters accept only integer keys, -so they never reach these columns. - -Compared with Spark, Comet prunes more on unannotated columns and less on annotated ones. Spark -pushes a filter only when the literal's type matches the column's Parquet type, so it doesn't push -`=` or `IN` on an unannotated `BINARY` column, though it does push `IS NULL`. On an annotated -column Spark also pushes ranges, which Comet keeps above the scan while the configuration is on. - -**Test coverage.** `ParquetReadV1Suite` writes raw Parquet files without Spark schema metadata -and verifies the inferred schema under both configuration values, Spark answer parity and -`CometNativeScanExec`, including a row containing invalid UTF-8 bytes and a projection that reads -only a non-string column. A second test reads two partition directories holding a valid value, -`0xff` and a null, and a Spark-written file, with row-filter pushdown off and on. It compares `=`, -`IN`, `NOT IN`, `IS [NOT] NULL`, a range, an `OR`, a self-join on the string column and an -`ORDER BY ... LIMIT` against Spark, and checks that `value = 'one'` reaches the row filter. Native -tests in `parquet_exec.rs` check which row groups each filter prunes, for an unannotated and an -annotated column, with and without row filters. They include `NOT IN`, a range and `U+FFFD` -literals, none of which may prune. A native test in `schema_adapter.rs` checks that the adapter -compares an annotated column directly for `NOT IN` as for `IN`, and leaves a filter with a null or -invalid UTF-8 literal unchanged. +The reader validates file conversions against Spark's original requested schema, before the +intermediate binary type can admit a conversion Spark rejects. In particular, fixed-length byte +arrays requested as strings fail at the first data-page request. Empty files and files discarded +by format pruning do not trigger a deferred conversion error, while row filtering cannot hide one. + +**Filter pushdown.** The scan rewrites `=`, `IN` and `IS [NOT] NULL` on a top-level string column +to raw-byte comparisons where safe, so `value = 'one'` becomes `value = X'6F6E65'`. A malformed +value decodes to a string containing `U+FFFD`, so equality and IN require literals without that +character. The expression adapter restores direct string comparisons for annotated columns. + +Other simple predicates, including ranges, `LIKE`, `NOT IN` and literals containing `U+FFFD`, +reach the reader with the same decoding expression used by the output projection. Function calls +remain above the scan to preserve their error behavior. On validated UTF8 columns, +removing that expression exposes the original predicate to statistics pruning. On unannotated +columns the decode remains, so a range cannot incorrectly prune using raw bytes whose ordering +may differ from decoded strings. Row filters can evaluate the decoded predicate. Spark's Filter +above the scan still re-evaluates every data filter. Join and TopK dynamic filters accept only +integer keys; a filter pushed onto the scan's string output is mapped through its decoding +projection in the same way. + +Compared with Spark, Comet can prune more on unannotated columns: Spark does not push string +`=` or `IN` literals onto an unannotated BINARY column, though it pushes null checks. Annotated +UTF8 columns in unencrypted files retain equality, range and LIKE pruning with the setting on. + +**Test coverage.** `ParquetReadV1Suite` covers inferred schemas, malformed bytes, native execution, +partitioned files, annotated strings, filters, joins and top-k, with row filtering off and on. It +also checks that an explicit string schema over fixed-length binary raises Spark's conversion +error, while an explicit binary schema reads successfully. Native tests check row-group pruning +for equality, ranges, LIKE, IN and NOT IN, decoded comparisons on malformed bytes, and mixed +annotated/unannotated files. They cover fixed-length binary errors with and without row filters, +including a filter that would discard every row, and the empty-file exception. Adapter and reader +tests check that decode removal requires a marker derived from the Parquet annotation, rather +than trusting an Arrow string hint or a marker supplied by the file. **Known divergences.** Decoding is not byte-preserving, so the differences described in [Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any diff --git a/docs/source/user-guide/latest/parquet.md b/docs/source/user-guide/latest/parquet.md index 466ed37c369..af7e3a0c5c7 100644 --- a/docs/source/user-guide/latest/parquet.md +++ b/docs/source/user-guide/latest/parquet.md @@ -211,11 +211,18 @@ page-index, and bloom-filter pruning happen regardless of this flag whenever Spa `spark.sql.parquet.filterPushdown` is on, except for filters that compare `FLOAT` or `DOUBLE` values, which do not prune while this flag is on (see [Floating-point Number Comparison](compatibility/floating-point.md)). -With `spark.sql.parquet.binaryAsString=true`, the native scan reads every top-level string column -as binary and decodes it after reading. Filters on those columns prune only for `=`, `IN` and -`IS [NOT] NULL` against a literal without `U+FFFD`, which the scan compares as raw bytes. Other -filters on them, such as ranges and `LIKE`, don't prune while the setting is on, even for columns -the file annotates as strings. +With `spark.sql.parquet.binaryAsString=true`, the native scan uses a binary read schema for +top-level string columns and decodes their bytes with replacement. For unencrypted files whose +columns carry the UTF8 annotation, the per-file adapter removes that decode and uses the validated +string column directly. Equality, range and `LIKE` filters retain their usual pruning on these +columns. On unannotated columns, `=`, `IN` and `IS [NOT] NULL` can prune using raw bytes when string +literals contain no `U+FFFD`. Other predicates compare decoded strings and cannot prune raw-byte +statistics, though they can filter rows when row filtering is enabled. Function-call filters stay +above the scan. Encrypted scans retain the +decode because the reader cannot rewrite their schema metadata while preserving decryption state. + +The binary read schema does not broaden Spark's supported conversions: reading a +`FIXED_LEN_BYTE_ARRAY` column as a string still raises a conversion error when data is decoded. ### Row Group Assignment to Splits diff --git a/native/core/src/parquet/eager_page_index_reader_factory.rs b/native/core/src/parquet/eager_page_index_reader_factory.rs index 00220e26666..4c3ea4ea3e2 100644 --- a/native/core/src/parquet/eager_page_index_reader_factory.rs +++ b/native/core/src/parquet/eager_page_index_reader_factory.rs @@ -77,6 +77,11 @@ //! a `TIMESTAMP(NANOS)` leaf even without them, a deferred conversion failure is raised on the //! first data-page request. Footer, Bloom-filter and page-index reads still proceed, so format //! pruning can discard a file without decoding it, but row selection cannot hide the failure. +//! Binary-as-string scans also validate the original requested types at that boundary, so an +//! intermediate Binary read cannot admit fixed-length binary requested as StringType. For +//! unencrypted opens, a per-open Arrow hint carries the raw UTF8 annotation to the expression +//! adapter. It can then remove synthetic decoding for validated columns in projections and +//! predicates. The shared cache keeps the original footer. //! //! With `with_next_file_metadata_prefetch`, readers also load the next file's metadata before //! DataFusion opens it. DataFusion's `FileStream` keeps one I/O outstanding and plans a file @@ -97,6 +102,7 @@ use crate::parquet::datetime_rebase::stamp_int96_leaves; use crate::parquet::parquet_support::SparkParquetOptions; use crate::parquet::schema_adapter::{ check_file_conversions, RejectOnNonEmpty, NANOS_TIMESTAMP_KEY, REPEATED_PRIMITIVE_KEY, + UTF8_VALIDATED_KEY, }; use arrow::datatypes::{DataType, FieldRef, Schema, SchemaRef}; use async_trait::async_trait; @@ -229,6 +235,7 @@ pub struct EagerPageIndexReaderFactory { // Enable the footer workaround only for scans that project Variant. // https://github.com/apache/datafusion-comet/issues/5477 spark_variant_schema: bool, + binary_as_string: bool, // Refuse a file whose Parquet schema carries no field id, as Spark's `ParquetReadSupport` // does when the requested schema carries one and `ignoreMissing` is not set. require_field_ids: bool, @@ -264,6 +271,7 @@ impl EagerPageIndexReaderFactory { metadata_cache, scan_io_metrics, spark_variant_schema: false, + binary_as_string: false, require_field_ids: false, stamp_int96_leaves: false, conversion_check: None, @@ -273,7 +281,8 @@ impl EagerPageIndexReaderFactory { } /// Retain raw LIST encoding and `TIMESTAMP(NANOS)` leaves for open-time validation. When row - /// filters are enabled, or the file has a `TIMESTAMP(NANOS)` leaf, enforce deferred failures + /// filters or binary-as-string reads are enabled, or the file has a `TIMESTAMP(NANOS)` leaf, + /// enforce deferred failures /// at the first data-page read, after format pruning. pub(crate) fn with_conversion_check( mut self, @@ -285,6 +294,11 @@ impl EagerPageIndexReaderFactory { self } + pub(crate) fn with_binary_as_string(mut self, enabled: bool) -> Self { + self.binary_as_string = enabled; + self + } + pub fn with_spark_variant_schema(mut self, enabled: bool) -> Self { self.spark_variant_schema = enabled; self @@ -370,6 +384,7 @@ impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { metadata_cache: Arc::clone(&self.metadata_cache), metadata_size_hint, spark_variant_schema: self.spark_variant_schema, + binary_as_string: self.binary_as_string, require_field_ids: self.require_field_ids, stamp_int96_leaves: self.stamp_int96_leaves, conversion_check: self.conversion_check.clone(), @@ -391,6 +406,7 @@ struct EagerPageIndexReader { metadata_cache: Arc, metadata_size_hint: Option, spark_variant_schema: bool, + binary_as_string: bool, require_field_ids: bool, stamp_int96_leaves: bool, conversion_check: Option<(SchemaRef, SparkParquetOptions, bool)>, @@ -529,6 +545,70 @@ fn with_spark_arrow_schema(metadata: Arc) -> ParquetResult, +) -> ParquetResult> { + let file = metadata.file_metadata(); + let schema = parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata())?; + let fields = schema + .fields() + .iter() + .zip(file.schema_descr().root_schema().get_fields()) + .map(|(field, parquet)| { + let validated = parquet.is_primitive() + && parquet.get_basic_info().converted_type() == ConvertedType::UTF8; + let mut metadata = field.metadata().clone(); + metadata.remove(UTF8_VALIDATED_KEY); + if validated { + metadata.insert(UTF8_VALIDATED_KEY.to_string(), "true".to_string()); + } + let mut field = field.as_ref().clone().with_metadata(metadata); + if !validated + && parquet.get_basic_info().converted_type() != ConvertedType::JSON + && matches!( + field.data_type(), + DataType::Utf8 | DataType::LargeUtf8 | DataType::Utf8View + ) + { + field = field.with_data_type(DataType::Binary); + } + Arc::new(field) + }) + .collect::>(); + let schema = Schema::new_with_metadata(fields, schema.metadata().clone()); + let mut key_values = file + .key_value_metadata() + .into_iter() + .flatten() + .filter(|kv| kv.key != ARROW_SCHEMA_META_KEY) + .cloned() + .collect::>(); + key_values.push(KeyValue::new( + ARROW_SCHEMA_META_KEY.to_string(), + encode_arrow_schema(&schema), + )); + let file = FileMetaData::new( + file.version(), + file.num_rows(), + file.created_by().map(str::to_owned), + Some(key_values), + file.schema_descr_ptr(), + file.column_orders().cloned(), + ); + Ok(Arc::new( + ParquetMetaDataBuilder::new(file) + .set_row_groups(metadata.row_groups().to_vec()) + .set_column_index(metadata.column_index().cloned()) + .set_offset_index(metadata.offset_index().cloned()) + .build(), + )) +} + /// Whether the file declares `column` as `TIMESTAMP(NANOS)`. fn is_nanos_timestamp(column: &ColumnDescPtr) -> bool { matches!( @@ -769,6 +849,11 @@ impl AsyncFileReader for EagerPageIndexReader { } else { metadata }; + let metadata = if self.binary_as_string && cache_enabled { + with_binary_as_string_schema(metadata)? + } else { + metadata + }; if let Some((required, conversion_options, row_filters)) = self.conversion_check.as_ref() { @@ -778,6 +863,7 @@ impl AsyncFileReader for EagerPageIndexReader { // `TIMESTAMP(NANOS)` rejection is deferred from here even without row filters. let has_nanos = columns.iter().any(is_nanos_timestamp); let needs_check = *row_filters + || self.binary_as_string || has_nanos || columns.iter().any(|column| { column.self_type().get_basic_info().repetition() @@ -789,10 +875,12 @@ impl AsyncFileReader for EagerPageIndexReader { let schema = parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata())?; let schema = mark_parquet_leaves(&schema, file.schema_descr()); + let mut conversion_options = conversion_options.clone(); + conversion_options.reject_fixed_binary_as_string = self.binary_as_string; let rejection = check_file_conversions( Arc::clone(required), Arc::new(schema), - conversion_options.clone(), + conversion_options, ) .map_err(|e| match e { // Keep structured Spark errors directly inside the Parquet wrapper so @@ -800,7 +888,7 @@ impl AsyncFileReader for EagerPageIndexReader { DataFusionError::External(source) => ParquetError::External(source), other => ParquetError::External(Box::new(other)), })?; - if *row_filters || has_nanos { + if *row_filters || self.binary_as_string || has_nanos { if let Some(rejection) = &rejection { self.deferred_rejections.lock().unwrap().insert( object_meta.location.clone(), @@ -2030,4 +2118,56 @@ mod tests { .expect("the metadata load in flight was not aborted"); assert_eq!(metric(&metrics, "scan_io_metadata_prefetches"), 0); } + + #[test] + fn binary_as_string_schema_uses_parquet_annotation_not_arrow_hint() { + use arrow::datatypes::Field; + use parquet::schema::parser::parse_message_type; + let parquet = Arc::new(parse_message_type( + "message test { OPTIONAL BINARY raw; OPTIONAL BINARY text (UTF8); OPTIONAL BINARY json (JSON); }").unwrap()); + let fields = ["raw", "text", "json"] + .into_iter() + .map(|name| { + Field::new(name, DataType::Utf8, true).with_metadata(HashMap::from([( + UTF8_VALIDATED_KEY.to_string(), + "true".to_string(), + )])) + }) + .collect::>(); + let hint = Schema::new(fields); + let file = FileMetaData::new( + 1, + 0, + None, + Some(vec![KeyValue::new( + ARROW_SCHEMA_META_KEY.to_string(), + encode_arrow_schema(&hint), + )]), + Arc::new(SchemaDescriptor::new(parquet)), + None, + ); + let original = Arc::new(ParquetMetaData::new(file, vec![])); + let rewritten = with_binary_as_string_schema(original.clone()).unwrap(); + let file = rewritten.file_metadata(); + let schema = + parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata()).unwrap(); + assert_eq!(schema.field(0).data_type(), &DataType::Binary); + assert_eq!(schema.field(1).data_type(), &DataType::Utf8); + assert_eq!(schema.field(2).data_type(), &DataType::Utf8); + for index in [0, 2] { + assert!(!schema + .field(index) + .metadata() + .contains_key(UTF8_VALIDATED_KEY)); + } + assert_eq!( + schema + .field(1) + .metadata() + .get(UTF8_VALIDATED_KEY) + .map(String::as_str), + Some("true") + ); + assert_eq!(rewritten.row_groups(), original.row_groups()); + } } diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index fcb628341b0..96388491a4b 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -25,7 +25,7 @@ use crate::parquet::parquet_support::{ }; use crate::parquet::schema_adapter::SparkPhysicalExprAdapterFactory; use arrow::datatypes::{DataType, Field, FieldRef, Schema, SchemaRef}; -use datafusion::common::tree_node::TreeNode; +use datafusion::common::tree_node::{Transformed, TransformedResult, TreeNode}; use datafusion::config::{ParquetOptions, TableParquetOptions}; use datafusion::datasource::listing::PartitionedFile; use datafusion::datasource::physical_plan::{ @@ -35,7 +35,7 @@ use datafusion::datasource::source::DataSourceExec; use datafusion::execution::object_store::ObjectStoreUrl; use datafusion::logical_expr::Operator; use datafusion::physical_expr::expressions::{ - in_list, BinaryExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, Literal, + in_list, BinaryExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, LikeExpr, Literal, }; use datafusion::physical_expr::projection::{ProjectionExpr, ProjectionExprs}; use datafusion::physical_expr::utils::collect_columns; @@ -115,32 +115,31 @@ fn references_binary_read_column(expr: &Arc, required_schema: .any(|column| reads_binary(column, required_schema)) } -/// Rewrites a data filter for a scan that reads string columns as Binary (see -/// `binary_as_string_read_schema`), or returns `None` when it can't go into the scan. +/// Rewrites simple comparisons against the Binary read schema, using raw bytes where exact +/// and the scan's replacement decoder otherwise. Function calls remain above the scan. /// /// The scan decodes each such column with replacement: a valid value decodes to itself, and an /// invalid one to a string containing U+FFFD. So for a literal without U+FFFD, `s = 'lit'` on the /// decoded string holds exactly when `s = X''` holds on the raw bytes, and the same -/// goes for `IN`. `IS [NOT] NULL` holds on both, because decoding keeps nulls. Other filters on -/// these columns stay above the scan. A range can disagree because decoding can reorder values: +/// goes for `IN`. `IS [NOT] NULL` holds on both, because decoding keeps nulls. Other filters +/// compare decoded values inside the scan. A raw-byte range can disagree because decoding can +/// reorder values: /// `"a\x80"` sorts below `"aé"` as bytes, but `"a\u{FFFD}"` sorts above it. -/// -/// Spark's Filter above the scan re-evaluates every data filter, so a pushed filter only has to -/// keep every row the original keeps. That's why an `AND` keeps whichever side can be pushed. fn binary_as_string_pushdown_filter( filter: &Arc, required_schema: &Schema, filter_read_schema: &Schema, + parquet_options: &SparkParquetOptions, ) -> Result>, ExecutionError> { if !references_binary_read_column(filter, required_schema) { return Ok(Some(Arc::clone(filter))); } - let reads_binary = |expr: &Arc| { + let is_binary_column = |expr: &Arc| { expr.downcast_ref::() .is_some_and(|column| reads_binary(column, required_schema)) }; let pushdown = |expr: &Arc| { - binary_as_string_pushdown_filter(expr, required_schema, filter_read_schema) + binary_as_string_pushdown_filter(expr, required_schema, filter_read_schema, parquet_options) }; if let Some(binary) = filter.downcast_ref::() { @@ -163,13 +162,13 @@ fn binary_as_string_pushdown_filter( }); } Operator::Eq => { - if reads_binary(left) { + if is_binary_column(left) { if let Some(literal) = binary_literal(right) { let eq = BinaryExpr::new(Arc::clone(left), Operator::Eq, literal); return Ok(Some(Arc::new(eq))); } } - if reads_binary(right) { + if is_binary_column(right) { if let Some(literal) = binary_literal(left) { let eq = BinaryExpr::new(literal, Operator::Eq, Arc::clone(right)); return Ok(Some(Arc::new(eq))); @@ -179,22 +178,72 @@ fn binary_as_string_pushdown_filter( _ => {} } } else if let Some(list) = filter.downcast_ref::() { - if !list.negated() && reads_binary(list.expr()) { + if !list.negated() && is_binary_column(list.expr()) { if let Some(values) = list.list().iter().map(binary_literal).collect() { let expr = Arc::clone(list.expr()); return Ok(Some(in_list(expr, values, &false, filter_read_schema)?)); } } } else if let Some(is_null) = filter.downcast_ref::() { - if reads_binary(is_null.arg()) { + if is_binary_column(is_null.arg()) { return Ok(Some(Arc::clone(filter))); } } else if let Some(is_not_null) = filter.downcast_ref::() { - if reads_binary(is_not_null.arg()) { + if is_binary_column(is_not_null.arg()) { return Ok(Some(Arc::clone(filter))); } } - Ok(None) + // Keep function evaluation above the scan. Parquet row filters flatten evaluation errors + // into Arrow errors, so moving a potentially throwing expression here changes its error. + let simple_comparison = filter.downcast_ref::().is_some_and(|binary| { + matches!( + binary.op(), + Operator::Eq + | Operator::NotEq + | Operator::Lt + | Operator::LtEq + | Operator::Gt + | Operator::GtEq + | Operator::IsDistinctFrom + | Operator::IsNotDistinctFrom + ) + }) || filter.downcast_ref::().is_some() + || filter.downcast_ref::().is_some(); + let simple_operands = filter.children().iter().all(|expr| { + expr.downcast_ref::().is_some() || expr.downcast_ref::().is_some() + }); + if !simple_comparison || !simple_operands { + return Ok(None); + } + // Keep other simple predicates over decoded strings. The per-file adapter can remove the decode + // for validated UTF8 columns, exposing ranges and LIKE to statistics pruning. Unannotated + // columns retain the decode, so row filters never compare malformed bytes as raw strings. + let decoded = Arc::clone(filter) + .transform(|expr| { + if expr + .downcast_ref::() + .is_some_and(|column| reads_binary(column, required_schema)) + { + Ok(Transformed::yes(binary_as_string_cast( + expr, + parquet_options, + ))) + } else { + Ok(Transformed::no(expr)) + } + }) + .data()?; + Ok(Some(decoded)) +} + +fn binary_as_string_cast( + child: Arc, + parquet_options: &SparkParquetOptions, +) -> Arc { + let mut options = SparkCastOptions::new(parquet_options.eval_mode, &parquet_options.timezone); + options.allow_cast_unsigned_ints = parquet_options.allow_cast_unsigned_ints; + options.is_adapting_schema = true; + Arc::new(Cast::new(child, DataType::Utf8, options, None, None)) } /// `expr` as a Binary literal holding the same bytes, if it is a string literal without U+FFFD. @@ -225,17 +274,7 @@ fn binary_as_string_projection( let expr = if read_field.data_type() == output_field.data_type() { child } else { - let mut cast_options = - SparkCastOptions::new(parquet_options.eval_mode, &parquet_options.timezone); - cast_options.allow_cast_unsigned_ints = parquet_options.allow_cast_unsigned_ints; - cast_options.is_adapting_schema = true; - Arc::new(Cast::new( - child, - output_field.data_type().clone(), - cast_options, - None, - None, - )) as Arc + binary_as_string_cast(child, parquet_options) }; ProjectionExpr::new(expr, output_field.name()) })) @@ -467,6 +506,7 @@ pub(crate) fn init_datasource_exec( scan_io_source, parquet_source.metrics(), ) + .with_binary_as_string(binary_as_string) .with_spark_variant_schema(projects_variant) .with_require_field_ids(require_field_ids) .with_int96_leaf_stamp(rebase_from_file_metadata) @@ -502,7 +542,7 @@ pub(crate) fn init_datasource_exec( }); // In binaryAsString mode the filters are typed against the Utf8 output schema, but the scan // reads the string columns as Binary. Rewrite the filters on those columns that can compare - // raw bytes, and keep the rest out of the scan. Filters on other columns are pushed as is. + // raw bytes, and map the rest through the decoder. Filters on other columns are unchanged. let data_filters = match data_filters { Some(filters) if binary_as_string => { // The planner binds data filters against the required columns followed by the @@ -521,6 +561,7 @@ pub(crate) fn init_datasource_exec( &filter, &required_schema, &filter_read_schema, + &spark_parquet_options, )? { pushable.push(filter); } @@ -558,7 +599,8 @@ pub(crate) fn init_datasource_exec( file_source }; let expr_adapter_factory: Arc = Arc::new( - SparkPhysicalExprAdapterFactory::new(spark_parquet_options, default_values), + SparkPhysicalExprAdapterFactory::new(spark_parquet_options, default_values) + .with_binary_as_string(binary_as_string && !encryption_enabled), ); let file_groups = file_groups @@ -807,6 +849,14 @@ mod tests { file: &PartitionedFile, filters: Vec>, session_ctx: &Arc, + ) -> Arc { + binary_as_string_files_scan(std::slice::from_ref(file), filters, session_ctx) + } + + fn binary_as_string_files_scan( + files: &[PartitionedFile], + filters: Vec>, + session_ctx: &Arc, ) -> Arc { let required_schema = binary_as_string_required_schema(); init_datasource_exec( @@ -815,7 +865,7 @@ mod tests { None, ObjectStoreUrl::local_filesystem(), ObjectStoreBackend::Local, - vec![vec![file.clone()]], + vec![files.to_vec()], Some(vec![0, 1]), Some(filters), None, @@ -947,8 +997,7 @@ mod tests { } /// An unannotated BINARY column read as a string prunes on `=`, `IN` and `IS [NOT] NULL` - /// compared as raw bytes, while filters that could disagree with the decoded strings stay - /// above the scan. + /// compared as raw bytes. Other predicates retain the decoder and only filter rows. #[tokio::test] async fn binary_as_string_pushes_raw_byte_filters_on_unannotated_binary() { let file = write_binary_as_string_fixture( @@ -971,10 +1020,10 @@ mod tests { Arc::new(IsNotNullExpr::new(s())), groups(&[Some("one"), Some("\u{fffd}")]), ), - // Pushed, this would prune the null group. It isn't, because a range on the decoded - // strings can disagree with the same range on the bytes. + // Decoded ranges cannot prune raw-byte statistics. Row filtering still applies + // the range to the decoded strings and removes the null group. ("range", compare_s(Operator::Gt, "a"), every_group.clone()), - // The scan pushes only `=`, `IN` and `IS [NOT] NULL`, so `NOT IN` stays above it. + // NOT IN retains its decode and can filter rows, but cannot prune byte statistics. ("NOT IN", s_in(&["one"], true), every_group.clone()), // The invalid group decodes to U+FFFD, so comparing the literal's bytes would prune a // group that matches. @@ -989,7 +1038,7 @@ mod tests { s_in(&["one", "\u{fffd}"], false), every_group, ), - // A filter on another column still prunes, and `AND` keeps the side that is pushed. + // A filter on another column still prunes inside an AND with a decoded predicate. ( "AND", Arc::new(BinaryExpr::new( @@ -1004,7 +1053,17 @@ mod tests { for row_filter in [false, true] { assert_eq!( scan_binary_as_string(&file, vec![Arc::clone(&filter)], row_filter).await, - expected, + if row_filter { + match name { + "range" | "IN with a U+FFFD literal" => { + groups(&[Some("one"), Some("\u{fffd}")]) + } + "NOT IN" | "U+FFFD literal" => groups(&[Some("\u{fffd}")]), + _ => expected.clone(), + } + } else { + expected.clone() + }, "{name}, row filter {row_filter}" ); } @@ -1037,8 +1096,8 @@ mod tests { ); } - /// A column the file annotates as a string is read as Binary too, and the adapter's cast back - /// would hide it from pruning. The adapter compares the column directly instead. + /// Annotated strings bypass the synthetic binary decode, keeping direct-column statistics + /// pruning for equality, ranges, LIKE, IN, NOT IN and null checks. #[tokio::test] async fn binary_as_string_prunes_annotated_strings() { let file = write_binary_as_string_fixture( @@ -1049,10 +1108,21 @@ mod tests { let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), ("IN", s_in(&["two"], false), groups(&[Some("two")])), + ("NOT IN", s_in(&["one"], true), groups(&[Some("two")])), ( - "NOT IN", - s_in(&["one"], true), - groups(&[Some("one"), Some("two"), None]), + "range", + compare_s(Operator::Lt, "two"), + groups(&[Some("one")]), + ), + ( + "LIKE", + Arc::new(datafusion::physical_expr::expressions::LikeExpr::new( + false, + false, + s(), + Arc::new(Literal::new(ScalarValue::Utf8(Some("on%".to_string())))), + )), + groups(&[Some("one")]), ), ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), ( @@ -1072,6 +1142,93 @@ mod tests { } } + #[tokio::test] + async fn binary_as_string_keeps_throwing_functions_above_scan() { + let file = + write_binary_as_string_fixture(DataType::Binary, [Some(b"one"), Some(&[0xff]), None]); + let cast: Arc = Arc::new(Cast::new( + Arc::new(Column::new("s", 1)), + DataType::Int32, + SparkCastOptions::new(EvalMode::Ansi, "UTC"), + None, + None, + )); + let filter: Arc = Arc::new(IsNullExpr::new(cast)); + for row_filter in [false, true] { + assert_eq!( + scan_binary_as_string(&file, vec![filter.clone()], row_filter).await, + groups(&[Some("one"), Some("\u{fffd}"), None]) + ); + } + } + + #[tokio::test] + async fn binary_as_string_mixed_files_keep_unannotated_decode() { + let annotated = + write_binary_as_string_fixture(DataType::Utf8, [Some(b"a"), Some(b"zzz"), None]); + let binary = + write_binary_as_string_fixture(DataType::Binary, [Some(b"a\x80"), Some(b"a"), None]); + for row_filter in [false, true] { + let session = binary_as_string_session(row_filter); + let scan = binary_as_string_files_scan( + &[annotated.clone(), binary.clone()], + vec![compare_s(Operator::Lt, "aé")], + &session, + ); + let mut expected = BTreeMap::from([(Some("a".to_string()), 1_000)]); + if !row_filter { + // Only the annotated file prunes. Binary a\x80 must be decoded before its + // range is evaluated, since its raw and decoded ordering disagree. + expected.insert(Some("a\u{fffd}".to_string()), 500); + expected.insert(None, 500); + } + assert_eq!(decoded_counts(&scan, &session).await, expected); + } + } + + #[tokio::test] + async fn binary_as_string_rejects_fixed_binary_requested_as_string() { + use futures::TryStreamExt; + let fixed = write_binary_as_string_fixture( + DataType::FixedSizeBinary(3), + [Some(b"one"), Some(b"two"), None], + ); + for row_filter in [false, true] { + let session = binary_as_string_session(row_filter); + for filters in [vec![], vec![compare_s(Operator::Eq, "missing")]] { + let scan = binary_as_string_scan(&fixed, filters, &session); + let error = scan + .execute(0, session.task_ctx()) + .unwrap() + .try_collect::>() + .await + .unwrap_err() + .to_string(); + assert!( + error.contains("FIXED_LEN_BYTE_ARRAY") && error.contains("string"), + "{error}" + ); + } + } + // An empty file never asks Spark's vectorized reader for a conversion updater. + let empty = datafusion_comet_spark_expr::test_common::file_util::get_temp_filename(); + let file = File::create(&empty).unwrap(); + let schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, false), + Field::new("s", DataType::FixedSizeBinary(3), true), + ])); + ArrowWriter::try_new(file, schema, None) + .unwrap() + .close() + .unwrap(); + let empty = PartitionedFile::from_path(empty.to_str().unwrap().to_string()).unwrap(); + for row_filter in [false, true] { + assert!(scan_binary_as_string(&empty, vec![], row_filter) + .await + .is_empty()); + } + } + fn write_scan_io_fixture() -> (String, SchemaRef) { let schema = Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, false), diff --git a/native/core/src/parquet/parquet_support.rs b/native/core/src/parquet/parquet_support.rs index 1a4601a7503..6b464e92757 100644 --- a/native/core/src/parquet/parquet_support.rs +++ b/native/core/src/parquet/parquet_support.rs @@ -127,6 +127,9 @@ pub struct SparkParquetOptions { /// resolution. Enabled by the Delta scan arm; the plain NativeScan keeps its documented /// no-rebase behavior (#5010). See `datetime_rebase.rs`. pub rebase_from_file_metadata: bool, + /// Check Spark Parquet's original StringType request before binaryAsString substitutes + /// Binary. Kept separate from Iceberg's UUID-to-string adaptation, which accepts fixed binary. + pub reject_fixed_binary_as_string: bool, /// Effective `spark.sql.parquet.datetimeRebaseModeInRead` (a `LegacyBehaviorPolicy` value), /// forwarded from the JVM at planning time. Consulted -- exactly like Spark's /// `DataSourceUtils.getRebaseSpec` `modeByConfig` fallback -- only for files whose footer @@ -152,6 +155,7 @@ impl SparkParquetOptions { allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, + reject_fixed_binary_as_string: false, rebase_from_file_metadata: false, datetime_rebase_mode_in_read: String::new(), int96_rebase_mode_in_read: String::new(), @@ -170,6 +174,7 @@ impl SparkParquetOptions { allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, + reject_fixed_binary_as_string: false, rebase_from_file_metadata: false, datetime_rebase_mode_in_read: String::new(), int96_rebase_mode_in_read: String::new(), diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index f4bf12da058..e929ab6b106 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -63,6 +63,7 @@ pub struct SparkPhysicalExprAdapterFactory { /// Default values for columns that may be missing from the physical schema. /// The key is the Column (containing name and index). default_values: Option>, + binary_as_string: bool, } impl SparkPhysicalExprAdapterFactory { @@ -74,8 +75,15 @@ impl SparkPhysicalExprAdapterFactory { Self { parquet_options, default_values, + binary_as_string: false, } } + + /// Elide the scan's synthetic decode only when the reader stamped the physical UTF8 field. + pub(crate) fn with_binary_as_string(mut self, enabled: bool) -> Self { + self.binary_as_string = enabled; + self + } } /// Returns true when casting `physical_type` to `target_type` is a *pure* structural @@ -389,6 +397,7 @@ fn parquet_primitive_name(dt: &DataType) -> &'static str { DataType::Int64 => "INT64", DataType::Float32 => "FLOAT", DataType::Float64 => "DOUBLE", + DataType::FixedSizeBinary(_) => "FIXED_LEN_BYTE_ARRAY", DataType::Utf8 | DataType::LargeUtf8 | DataType::Binary | DataType::LargeBinary => "BINARY", // Spark stores DATE as INT32 with a DATE logical-type annotation. DataType::Date32 => "INT32", @@ -549,6 +558,18 @@ fn spark_has_updater( options: &SparkParquetOptions, nanos_timestamp: bool, ) -> bool { + // FIXED_LEN_BYTE_ARRAY has a Binary/Decimal updater, but no String updater. Validate the + // original requested type even when binaryAsString later reads through an intermediate Binary. + if options.reject_fixed_binary_as_string + && matches!(physical_type, DataType::FixedSizeBinary(_)) + && matches!( + target_type, + DataType::Utf8 | DataType::LargeUtf8 | DataType::Utf8View + ) + { + return false; + } + // Reject reading a string/binary Parquet column as anything else. Spark's // `ParquetVectorUpdaterFactory.getUpdater` BINARY case allows StringType / // BinaryType, or DecimalType only when the column carries a @@ -1116,6 +1137,7 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { physical_file_schema: adapted_physical_schema, parquet_options: self.parquet_options.clone(), default_values: self.default_values.clone(), + binary_as_string: self.binary_as_string, default_adapter, logical_to_physical_names, original_physical_dup_check, @@ -1146,6 +1168,7 @@ struct SparkPhysicalExprAdapter { default_values: Option>, /// The default DataFusion adapter to delegate standard handling to default_adapter: Arc, + binary_as_string: bool, /// Mapping from logical column names to original physical column names, /// used for case-insensitive mode where names differ in casing. /// After the default adapter rewrites expressions using the remapped @@ -1206,6 +1229,8 @@ impl PhysicalExprAdapter for SparkPhysicalExprAdapter { // - CometCastColumnExpr (for Struct/List/Map, uses spark_parquet_convert) // - Spark Cast (for simple scalar types) rewritten + .transform(|e| self.unwrap_binary_as_string_cast(e)) + .data()? .transform(|e| self.unwrap_cast_to_binary_comparison(e)) .data()? .transform(|e| self.replace_with_spark_cast(e)) @@ -1432,6 +1457,36 @@ impl SparkPhysicalExprAdapter { .data() } + /// The reader identifies columns whose Parquet UTF8 annotation makes arrow-rs validate the + /// bytes. On those columns, decode(CAST(s AS Binary)) is identity. Run before replacing the + /// adapter's CastExpr so both projections and predicates can expose the original column. + fn unwrap_binary_as_string_cast( + &self, + expr: Arc, + ) -> DataFusionResult>> { + if self.binary_as_string { + if let Some(decode) = expr.downcast_ref::() { + if decode.data_type == DataType::Utf8 && decode.cast_options.is_adapting_schema { + if let Some((column, DataType::Utf8)) = + self.column_cast_to_binary(&decode.child) + { + let index = column.downcast_ref::().unwrap().index(); + if self + .physical_file_schema + .field(index) + .metadata() + .get(UTF8_VALIDATED_KEY) + .is_some_and(|value| value == "true") + { + return Ok(Transformed::yes(column)); + } + } + } + } + } + Ok(Transformed::no(expr)) + } + /// 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'..'` @@ -1757,6 +1812,9 @@ impl SparkPhysicalExprAdapter { } } +/// Private field hint derived from the raw footer, never trusted from an input file's Arrow hint. +pub(crate) const UTF8_VALIDATED_KEY: &str = "comet.parquet.utf8_validated"; + /// Private, in-memory hint used only while checking the raw footer's LIST encoding. pub(crate) const REPEATED_PRIMITIVE_KEY: &str = "comet.parquet.repeated_primitive"; @@ -1908,7 +1966,7 @@ pub(crate) mod test { use crate::parquet::parquet_support::SparkParquetOptions; use crate::parquet::schema_adapter::{ check_conversion, check_field_conversion, is_pure_structural_narrowing, ConversionCheck, - SparkPhysicalExprAdapterFactory, NANOS_TIMESTAMP_KEY, + SparkPhysicalExprAdapterFactory, NANOS_TIMESTAMP_KEY, UTF8_VALIDATED_KEY, }; use arrow::array::cast::AsArray; use arrow::array::UInt32Array; @@ -1935,7 +1993,7 @@ pub(crate) mod test { use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_plan::{ExecutionPlan, SendableRecordBatchStream}; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; - use datafusion_comet_spark_expr::EvalMode; + use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions}; use datafusion_physical_expr_adapter::PhysicalExprAdapterFactory; use futures::StreamExt; use parquet::arrow::ArrowWriter; @@ -5685,4 +5743,80 @@ pub(crate) mod test { ); } } + + #[test] + fn binary_as_string_decode_elision_requires_reader_validation() { + let logical = Arc::new(Schema::new(vec![Field::new("s", DataType::Binary, true)])); + for validated in [false, true] { + let metadata = if validated { + HashMap::from([(UTF8_VALIDATED_KEY.to_string(), "true".to_string())]) + } else { + HashMap::new() + }; + let physical = Arc::new(Schema::new(vec![ + Field::new("s", DataType::Utf8, true).with_metadata(metadata) + ])); + let adapter = SparkPhysicalExprAdapterFactory::new( + SparkParquetOptions::new(EvalMode::Legacy, "UTC"), + None, + ) + .with_binary_as_string(true) + .create(logical.clone(), physical) + .unwrap(); + let mut options = SparkCastOptions::new(EvalMode::Legacy, "UTC"); + options.is_adapting_schema = true; + let decode: Arc = Arc::new(Cast::new( + Arc::new(Column::new("s", 0)), + DataType::Utf8, + options, + None, + None, + )); + let rewritten = adapter.rewrite(decode).unwrap(); + assert_eq!(rewritten.downcast_ref::().is_some(), validated); + let range = Arc::new(BinaryExpr::new( + rewritten, + Operator::Lt, + Arc::new(Literal::new(ScalarValue::Utf8(Some("two".to_string())))), + )) as Arc; + let batch = RecordBatch::try_new( + Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, true)])), + vec![Arc::new(StringArray::from(vec![ + Some("one"), + Some("two"), + None, + ]))], + ) + .unwrap(); + let result = range.evaluate(&batch).unwrap().into_array(3).unwrap(); + assert_eq!( + result.as_boolean().iter().collect::>(), + vec![Some(true), Some(false), None] + ); + } + } + #[test] + fn binary_as_string_conversion_check_keeps_iceberg_uuid_adaptation() { + let physical = Arc::new(Schema::new(vec![Field::new( + "s", + DataType::FixedSizeBinary(16), + true, + )])); + let logical = Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, true)])); + let adapter = SparkPhysicalExprAdapterFactory::new( + SparkParquetOptions::new(EvalMode::Legacy, "UTC"), + None, + ) + .create(logical, physical.clone()) + .unwrap(); + let expr = adapter.rewrite(Arc::new(Column::new("s", 0))).unwrap(); + let uuid = uuid::Uuid::from_bytes([42; 16]); + let values = arrow::array::FixedSizeBinaryArray::try_from_iter( + [uuid.as_bytes().as_slice()].into_iter(), + ) + .unwrap(); + let batch = RecordBatch::try_new(physical, vec![Arc::new(values)]).unwrap(); + let values = expr.evaluate(&batch).unwrap().into_array(1).unwrap(); + assert_eq!(values.as_string::().value(0), uuid.to_string()); + } } diff --git a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala index c9db74310dc..4bb1a45eebc 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -359,8 +359,40 @@ abstract class ParquetReadSuite extends CometTestBase { } } + test("binaryAsString preserves fixed-length binary conversion errors") { + withTempDir { dir => + val path = new Path(dir.toURI.toString, "fixed.parquet") + val schema = MessageTypeParser.parseMessageType( + "message root { optional fixed_len_byte_array(3) value; }") + val writer = createParquetWriter(schema, path) + val record = new SimpleGroup(schema) + record.add(0, Binary.fromString("one")) + writer.write(record) + writer.close() + Seq("false", "true").foreach { rowFilter => + withSQLConf( + SQLConf.PARQUET_BINARY_AS_STRING.key -> "true", + CometConf.COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED.key -> rowFilter) { + withSQLConf(CometConf.COMET_ENABLED.key -> "false") { + intercept[SparkException] { + spark.read.schema("value string").parquet(path.toString).collect() + } + } + val df = spark.read.schema("value string").parquet(path.toString) + assert(collect(df.queryExecution.executedPlan) { case _: CometNativeScanExec => + true + }.nonEmpty) + val error = intercept[SparkException](df.collect()) + assert( + causeChain(error).exists(_.isInstanceOf[SchemaColumnConvertNotSupportedException])) + checkSparkAnswerAndOperator(spark.read.schema("value binary").parquet(path.toString)) + } + } + } + } + // With binaryAsString, the native scan compares `=`, `IN` and `IS [NOT] NULL` on the raw bytes - // and keeps other string filters above the scan. Spark compares raw bytes too, so these queries + // and evaluates other string filters on decoded values. Spark compares raw bytes, so these queries // match it. A query that tells a malformed value apart from U+FFFD doesn't, as the // compatibility guide describes. test("spark.sql.parquet.binaryAsString filters, joins and top-k") { @@ -409,6 +441,8 @@ abstract class ParquetReadSuite extends CometTestBase { "SELECT * FROM t WHERE value > 'o'", "SELECT * FROM t WHERE value = 'one' OR id = 21", "SELECT * FROM annotated WHERE value = 'one'", + "SELECT * FROM annotated WHERE value < 'two'", + "SELECT * FROM annotated WHERE value LIKE 'on%'", "SELECT * FROM annotated WHERE value IS NULL").foreach { query => val (_, plan) = checkSparkAnswerAndOperator(sql(query)) assertNativeScan(plan) diff --git a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala index a6f7bbe02d2..d63a3832ab7 100644 --- a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala +++ b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala @@ -425,9 +425,8 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { /** * Spark-written strings carry the UTF8 annotation. With `spark.sql.parquet.binaryAsString`, the - * native scan still reads them as binary and decodes them, and only `=`, `IN` and `IS [NOT] - * NULL` on them reach the reader. This compares a full scan and a selective filter with the - * setting off and on. + * native scan's per-file adapter removes the synthetic binary decode for these columns. Compare + * full scans, equality, ranges and LIKE pruning with the setting off and on. */ def binaryAsStringScanBenchmark(values: Int): Unit = { withTempPath { dir => @@ -441,7 +440,11 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { Seq( "Annotated String Scan" -> "SELECT sum(length(c1)) FROM parquetV1Table", "Annotated String Filter" -> - "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 = '500'").foreach { + "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 = '500'", + "Annotated String Range" -> + "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 < '100'", + "Annotated String LIKE" -> + "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 LIKE '500%'").foreach { case (name, query) => val benchmark = new Benchmark(name, values.toLong, output = output) Seq("false", "true").foreach { binaryAsString => From a18390f45aaf1d16baf816ba6aeb064a32711d85 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Sun, 8 Nov 2026 01:42:24 +0900 Subject: [PATCH 09/10] fix: retain only binaryAsString conversion validation --- .../spark_configs_support.md | 77 +++---- docs/source/user-guide/latest/parquet.md | 17 +- .../eager_page_index_reader_factory.rs | 151 +------------ native/core/src/parquet/parquet_exec.rs | 205 ++++-------------- native/core/src/parquet/schema_adapter.rs | 100 +-------- .../comet/parquet/ParquetReadSuite.scala | 4 +- .../sql/benchmark/CometReadBenchmark.scala | 11 +- 7 files changed, 101 insertions(+), 464 deletions(-) diff --git a/docs/source/contributor-guide/spark_configs_support.md b/docs/source/contributor-guide/spark_configs_support.md index ff8730b93bf..944813c2a57 100644 --- a/docs/source/contributor-guide/spark_configs_support.md +++ b/docs/source/contributor-guide/spark_configs_support.md @@ -166,49 +166,44 @@ configuration does not change the types inferred for those files. The inference `ParquetToSparkSchemaConverter` and the `BINARY` cases of `ParquetFilters` are the same in every Spark version listed above. -**Comet status.** Comet honors the configuration in the native Parquet scan. When enabled, -its logical read schema uses Arrow `Binary` for top-level strings and a projection decodes those -bytes to valid UTF-8. For unencrypted files, the reader derives a private field marker from each -column's actual Parquet UTF8 annotation. The expression adapter uses that marker to remove the -synthetic binary-to-string round trip for validated string columns. An Arrow `Utf8` type alone is -not enough: JSON and an `ARROW:schema` string hint over unannotated BINARY do not imply validation. -Encrypted scans retain the decode because rebuilding the footer would lose private decryption -state in the pinned Parquet API. The default `false` continues to infer unannotated BINARY as +**Comet status.** Comet honors the configuration in the native Parquet scan. When it is +enabled, the scan reads every top-level string column as Arrow `Binary` and decodes it to valid +UTF-8 at the scan boundary using Spark-compatible replacement semantics. Spark's schema doesn't +say which file columns carry the UTF8 annotation, so annotated columns take the same path. +arrow-rs validates UTF-8 only for Parquet columns annotated as strings, so decoding unannotated +columns straight into Arrow `Utf8` would put invalid UTF-8 into a string array, which is undefined +behavior. The default `false` value continues to return unannotated `BINARY` columns as `BinaryType`. -The reader validates file conversions against Spark's original requested schema, before the -intermediate binary type can admit a conversion Spark rejects. In particular, fixed-length byte -arrays requested as strings fail at the first data-page request. Empty files and files discarded -by format pruning do not trigger a deferred conversion error, while row filtering cannot hide one. - -**Filter pushdown.** The scan rewrites `=`, `IN` and `IS [NOT] NULL` on a top-level string column -to raw-byte comparisons where safe, so `value = 'one'` becomes `value = X'6F6E65'`. A malformed -value decodes to a string containing `U+FFFD`, so equality and IN require literals without that -character. The expression adapter restores direct string comparisons for annotated columns. - -Other simple predicates, including ranges, `LIKE`, `NOT IN` and literals containing `U+FFFD`, -reach the reader with the same decoding expression used by the output projection. Function calls -remain above the scan to preserve their error behavior. On validated UTF8 columns, -removing that expression exposes the original predicate to statistics pruning. On unannotated -columns the decode remains, so a range cannot incorrectly prune using raw bytes whose ordering -may differ from decoded strings. Row filters can evaluate the decoded predicate. Spark's Filter -above the scan still re-evaluates every data filter. Join and TopK dynamic filters accept only -integer keys; a filter pushed onto the scan's string output is mapped through its decoding -projection in the same way. - -Compared with Spark, Comet can prune more on unannotated columns: Spark does not push string -`=` or `IN` literals onto an unannotated BINARY column, though it pushes null checks. Annotated -UTF8 columns in unencrypted files retain equality, range and LIKE pruning with the setting on. - -**Test coverage.** `ParquetReadV1Suite` covers inferred schemas, malformed bytes, native execution, -partitioned files, annotated strings, filters, joins and top-k, with row filtering off and on. It -also checks that an explicit string schema over fixed-length binary raises Spark's conversion -error, while an explicit binary schema reads successfully. Native tests check row-group pruning -for equality, ranges, LIKE, IN and NOT IN, decoded comparisons on malformed bytes, and mixed -annotated/unannotated files. They cover fixed-length binary errors with and without row filters, -including a filter that would discard every row, and the empty-file exception. Adapter and reader -tests check that decode removal requires a marker derived from the Parquet annotation, rather -than trusting an Arrow string hint or a marker supplied by the file. +**Filter pushdown.** While the configuration is enabled, the scan rewrites `=`, `IN` and +`IS [NOT] NULL` on a top-level string column to compare raw bytes, so `value = 'one'` becomes +`value = X'6F6E65'`. A valid value decodes to itself, and a malformed one to a string containing +`U+FFFD`, so for a literal without `U+FFFD` the rewritten filter keeps exactly the rows the +original keeps. For a file that annotates the column as a string, the expression adapter compares +the column with the string literal directly, so the file's statistics still prune. Other filters +on these columns stay above the scan, because decoding can reorder values: `"a\x80"` sorts below +`"aé"` as bytes, but `"a\uFFFD"` sorts above it. That covers ranges, `LIKE`, function calls and +literals containing `U+FFFD`. Spark's `Filter` above the scan re-evaluates every data filter, so +results don't depend on what is pushed. Join and TopK dynamic filters accept only integer keys, +so they never reach these columns. + +Compared with Spark, Comet prunes more on unannotated columns and less on annotated ones. Spark +pushes a filter only when the literal's type matches the column's Parquet type, so it doesn't push +`=` or `IN` on an unannotated `BINARY` column, though it does push `IS NULL`. On an annotated +column Spark also pushes ranges, which Comet keeps above the scan while the configuration is on. + +**Test coverage.** `ParquetReadV1Suite` writes raw Parquet files without Spark schema metadata +and verifies the inferred schema under both configuration values, Spark answer parity and +`CometNativeScanExec`, including a row containing invalid UTF-8 bytes and a projection that reads +only a non-string column. A second test reads two partition directories holding a valid value, +`0xff` and a null, and a Spark-written file, with row-filter pushdown off and on. It compares `=`, +`IN`, `NOT IN`, `IS [NOT] NULL`, a range, an `OR`, a self-join on the string column and an +`ORDER BY ... LIMIT` against Spark, and checks that `value = 'one'` reaches the row filter. Native +tests in `parquet_exec.rs` check which row groups each filter prunes, for an unannotated and an +annotated column, with and without row filters. They include `NOT IN`, a range and `U+FFFD` +literals, none of which may prune. A native test in `schema_adapter.rs` checks that the adapter +compares an annotated column directly for `NOT IN` as for `IN`, and leaves a filter with a null or +invalid UTF-8 literal unchanged. **Known divergences.** Decoding is not byte-preserving, so the differences described in [Strings with non-UTF-8 bytes](../user-guide/latest/compatibility/index.md) apply to any diff --git a/docs/source/user-guide/latest/parquet.md b/docs/source/user-guide/latest/parquet.md index af7e3a0c5c7..466ed37c369 100644 --- a/docs/source/user-guide/latest/parquet.md +++ b/docs/source/user-guide/latest/parquet.md @@ -211,18 +211,11 @@ page-index, and bloom-filter pruning happen regardless of this flag whenever Spa `spark.sql.parquet.filterPushdown` is on, except for filters that compare `FLOAT` or `DOUBLE` values, which do not prune while this flag is on (see [Floating-point Number Comparison](compatibility/floating-point.md)). -With `spark.sql.parquet.binaryAsString=true`, the native scan uses a binary read schema for -top-level string columns and decodes their bytes with replacement. For unencrypted files whose -columns carry the UTF8 annotation, the per-file adapter removes that decode and uses the validated -string column directly. Equality, range and `LIKE` filters retain their usual pruning on these -columns. On unannotated columns, `=`, `IN` and `IS [NOT] NULL` can prune using raw bytes when string -literals contain no `U+FFFD`. Other predicates compare decoded strings and cannot prune raw-byte -statistics, though they can filter rows when row filtering is enabled. Function-call filters stay -above the scan. Encrypted scans retain the -decode because the reader cannot rewrite their schema metadata while preserving decryption state. - -The binary read schema does not broaden Spark's supported conversions: reading a -`FIXED_LEN_BYTE_ARRAY` column as a string still raises a conversion error when data is decoded. +With `spark.sql.parquet.binaryAsString=true`, the native scan reads every top-level string column +as binary and decodes it after reading. Filters on those columns prune only for `=`, `IN` and +`IS [NOT] NULL` against a literal without `U+FFFD`, which the scan compares as raw bytes. Other +filters on them, such as ranges and `LIKE`, don't prune while the setting is on, even for columns +the file annotates as strings. ### Row Group Assignment to Splits diff --git a/native/core/src/parquet/eager_page_index_reader_factory.rs b/native/core/src/parquet/eager_page_index_reader_factory.rs index 4c3ea4ea3e2..a67cc826fc9 100644 --- a/native/core/src/parquet/eager_page_index_reader_factory.rs +++ b/native/core/src/parquet/eager_page_index_reader_factory.rs @@ -77,11 +77,6 @@ //! a `TIMESTAMP(NANOS)` leaf even without them, a deferred conversion failure is raised on the //! first data-page request. Footer, Bloom-filter and page-index reads still proceed, so format //! pruning can discard a file without decoding it, but row selection cannot hide the failure. -//! Binary-as-string scans also validate the original requested types at that boundary, so an -//! intermediate Binary read cannot admit fixed-length binary requested as StringType. For -//! unencrypted opens, a per-open Arrow hint carries the raw UTF8 annotation to the expression -//! adapter. It can then remove synthetic decoding for validated columns in projections and -//! predicates. The shared cache keeps the original footer. //! //! With `with_next_file_metadata_prefetch`, readers also load the next file's metadata before //! DataFusion opens it. DataFusion's `FileStream` keeps one I/O outstanding and plans a file @@ -102,7 +97,6 @@ use crate::parquet::datetime_rebase::stamp_int96_leaves; use crate::parquet::parquet_support::SparkParquetOptions; use crate::parquet::schema_adapter::{ check_file_conversions, RejectOnNonEmpty, NANOS_TIMESTAMP_KEY, REPEATED_PRIMITIVE_KEY, - UTF8_VALIDATED_KEY, }; use arrow::datatypes::{DataType, FieldRef, Schema, SchemaRef}; use async_trait::async_trait; @@ -235,7 +229,6 @@ pub struct EagerPageIndexReaderFactory { // Enable the footer workaround only for scans that project Variant. // https://github.com/apache/datafusion-comet/issues/5477 spark_variant_schema: bool, - binary_as_string: bool, // Refuse a file whose Parquet schema carries no field id, as Spark's `ParquetReadSupport` // does when the requested schema carries one and `ignoreMissing` is not set. require_field_ids: bool, @@ -271,7 +264,6 @@ impl EagerPageIndexReaderFactory { metadata_cache, scan_io_metrics, spark_variant_schema: false, - binary_as_string: false, require_field_ids: false, stamp_int96_leaves: false, conversion_check: None, @@ -280,10 +272,9 @@ impl EagerPageIndexReaderFactory { } } - /// Retain raw LIST encoding and `TIMESTAMP(NANOS)` leaves for open-time validation. When row - /// filters or binary-as-string reads are enabled, or the file has a `TIMESTAMP(NANOS)` leaf, - /// enforce deferred failures - /// at the first data-page read, after format pruning. + /// Retain raw LIST encoding and `TIMESTAMP(NANOS)` leaves for open-time validation. + /// Row filters, binary-as-string checks and `TIMESTAMP(NANOS)` defer failures until + /// the first data-page read, after format pruning. pub(crate) fn with_conversion_check( mut self, required: SchemaRef, @@ -294,11 +285,6 @@ impl EagerPageIndexReaderFactory { self } - pub(crate) fn with_binary_as_string(mut self, enabled: bool) -> Self { - self.binary_as_string = enabled; - self - } - pub fn with_spark_variant_schema(mut self, enabled: bool) -> Self { self.spark_variant_schema = enabled; self @@ -384,7 +370,6 @@ impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { metadata_cache: Arc::clone(&self.metadata_cache), metadata_size_hint, spark_variant_schema: self.spark_variant_schema, - binary_as_string: self.binary_as_string, require_field_ids: self.require_field_ids, stamp_int96_leaves: self.stamp_int96_leaves, conversion_check: self.conversion_check.clone(), @@ -406,7 +391,6 @@ struct EagerPageIndexReader { metadata_cache: Arc, metadata_size_hint: Option, spark_variant_schema: bool, - binary_as_string: bool, require_field_ids: bool, stamp_int96_leaves: bool, conversion_check: Option<(SchemaRef, SparkParquetOptions, bool)>, @@ -545,70 +529,6 @@ fn with_spark_arrow_schema(metadata: Arc) -> ParquetResult, -) -> ParquetResult> { - let file = metadata.file_metadata(); - let schema = parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata())?; - let fields = schema - .fields() - .iter() - .zip(file.schema_descr().root_schema().get_fields()) - .map(|(field, parquet)| { - let validated = parquet.is_primitive() - && parquet.get_basic_info().converted_type() == ConvertedType::UTF8; - let mut metadata = field.metadata().clone(); - metadata.remove(UTF8_VALIDATED_KEY); - if validated { - metadata.insert(UTF8_VALIDATED_KEY.to_string(), "true".to_string()); - } - let mut field = field.as_ref().clone().with_metadata(metadata); - if !validated - && parquet.get_basic_info().converted_type() != ConvertedType::JSON - && matches!( - field.data_type(), - DataType::Utf8 | DataType::LargeUtf8 | DataType::Utf8View - ) - { - field = field.with_data_type(DataType::Binary); - } - Arc::new(field) - }) - .collect::>(); - let schema = Schema::new_with_metadata(fields, schema.metadata().clone()); - let mut key_values = file - .key_value_metadata() - .into_iter() - .flatten() - .filter(|kv| kv.key != ARROW_SCHEMA_META_KEY) - .cloned() - .collect::>(); - key_values.push(KeyValue::new( - ARROW_SCHEMA_META_KEY.to_string(), - encode_arrow_schema(&schema), - )); - let file = FileMetaData::new( - file.version(), - file.num_rows(), - file.created_by().map(str::to_owned), - Some(key_values), - file.schema_descr_ptr(), - file.column_orders().cloned(), - ); - Ok(Arc::new( - ParquetMetaDataBuilder::new(file) - .set_row_groups(metadata.row_groups().to_vec()) - .set_column_index(metadata.column_index().cloned()) - .set_offset_index(metadata.offset_index().cloned()) - .build(), - )) -} - /// Whether the file declares `column` as `TIMESTAMP(NANOS)`. fn is_nanos_timestamp(column: &ColumnDescPtr) -> bool { matches!( @@ -849,11 +769,6 @@ impl AsyncFileReader for EagerPageIndexReader { } else { metadata }; - let metadata = if self.binary_as_string && cache_enabled { - with_binary_as_string_schema(metadata)? - } else { - metadata - }; if let Some((required, conversion_options, row_filters)) = self.conversion_check.as_ref() { @@ -863,7 +778,7 @@ impl AsyncFileReader for EagerPageIndexReader { // `TIMESTAMP(NANOS)` rejection is deferred from here even without row filters. let has_nanos = columns.iter().any(is_nanos_timestamp); let needs_check = *row_filters - || self.binary_as_string + || conversion_options.reject_fixed_binary_as_string || has_nanos || columns.iter().any(|column| { column.self_type().get_basic_info().repetition() @@ -875,12 +790,10 @@ impl AsyncFileReader for EagerPageIndexReader { let schema = parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata())?; let schema = mark_parquet_leaves(&schema, file.schema_descr()); - let mut conversion_options = conversion_options.clone(); - conversion_options.reject_fixed_binary_as_string = self.binary_as_string; let rejection = check_file_conversions( Arc::clone(required), Arc::new(schema), - conversion_options, + conversion_options.clone(), ) .map_err(|e| match e { // Keep structured Spark errors directly inside the Parquet wrapper so @@ -888,7 +801,7 @@ impl AsyncFileReader for EagerPageIndexReader { DataFusionError::External(source) => ParquetError::External(source), other => ParquetError::External(Box::new(other)), })?; - if *row_filters || self.binary_as_string || has_nanos { + if *row_filters || conversion_options.reject_fixed_binary_as_string || has_nanos { if let Some(rejection) = &rejection { self.deferred_rejections.lock().unwrap().insert( object_meta.location.clone(), @@ -2118,56 +2031,4 @@ mod tests { .expect("the metadata load in flight was not aborted"); assert_eq!(metric(&metrics, "scan_io_metadata_prefetches"), 0); } - - #[test] - fn binary_as_string_schema_uses_parquet_annotation_not_arrow_hint() { - use arrow::datatypes::Field; - use parquet::schema::parser::parse_message_type; - let parquet = Arc::new(parse_message_type( - "message test { OPTIONAL BINARY raw; OPTIONAL BINARY text (UTF8); OPTIONAL BINARY json (JSON); }").unwrap()); - let fields = ["raw", "text", "json"] - .into_iter() - .map(|name| { - Field::new(name, DataType::Utf8, true).with_metadata(HashMap::from([( - UTF8_VALIDATED_KEY.to_string(), - "true".to_string(), - )])) - }) - .collect::>(); - let hint = Schema::new(fields); - let file = FileMetaData::new( - 1, - 0, - None, - Some(vec![KeyValue::new( - ARROW_SCHEMA_META_KEY.to_string(), - encode_arrow_schema(&hint), - )]), - Arc::new(SchemaDescriptor::new(parquet)), - None, - ); - let original = Arc::new(ParquetMetaData::new(file, vec![])); - let rewritten = with_binary_as_string_schema(original.clone()).unwrap(); - let file = rewritten.file_metadata(); - let schema = - parquet_to_arrow_schema(file.schema_descr(), file.key_value_metadata()).unwrap(); - assert_eq!(schema.field(0).data_type(), &DataType::Binary); - assert_eq!(schema.field(1).data_type(), &DataType::Utf8); - assert_eq!(schema.field(2).data_type(), &DataType::Utf8); - for index in [0, 2] { - assert!(!schema - .field(index) - .metadata() - .contains_key(UTF8_VALIDATED_KEY)); - } - assert_eq!( - schema - .field(1) - .metadata() - .get(UTF8_VALIDATED_KEY) - .map(String::as_str), - Some("true") - ); - assert_eq!(rewritten.row_groups(), original.row_groups()); - } } diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index 96388491a4b..21d30d3dbd6 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -25,7 +25,7 @@ use crate::parquet::parquet_support::{ }; use crate::parquet::schema_adapter::SparkPhysicalExprAdapterFactory; use arrow::datatypes::{DataType, Field, FieldRef, Schema, SchemaRef}; -use datafusion::common::tree_node::{Transformed, TransformedResult, TreeNode}; +use datafusion::common::tree_node::TreeNode; use datafusion::config::{ParquetOptions, TableParquetOptions}; use datafusion::datasource::listing::PartitionedFile; use datafusion::datasource::physical_plan::{ @@ -35,7 +35,7 @@ use datafusion::datasource::source::DataSourceExec; use datafusion::execution::object_store::ObjectStoreUrl; use datafusion::logical_expr::Operator; use datafusion::physical_expr::expressions::{ - in_list, BinaryExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, LikeExpr, Literal, + in_list, BinaryExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, Literal, }; use datafusion::physical_expr::projection::{ProjectionExpr, ProjectionExprs}; use datafusion::physical_expr::utils::collect_columns; @@ -115,31 +115,32 @@ fn references_binary_read_column(expr: &Arc, required_schema: .any(|column| reads_binary(column, required_schema)) } -/// Rewrites simple comparisons against the Binary read schema, using raw bytes where exact -/// and the scan's replacement decoder otherwise. Function calls remain above the scan. +/// Rewrites a data filter for a scan that reads string columns as Binary (see +/// `binary_as_string_read_schema`), or returns `None` when it can't go into the scan. /// /// The scan decodes each such column with replacement: a valid value decodes to itself, and an /// invalid one to a string containing U+FFFD. So for a literal without U+FFFD, `s = 'lit'` on the /// decoded string holds exactly when `s = X''` holds on the raw bytes, and the same -/// goes for `IN`. `IS [NOT] NULL` holds on both, because decoding keeps nulls. Other filters -/// compare decoded values inside the scan. A raw-byte range can disagree because decoding can -/// reorder values: +/// goes for `IN`. `IS [NOT] NULL` holds on both, because decoding keeps nulls. Other filters on +/// these columns stay above the scan. A range can disagree because decoding can reorder values: /// `"a\x80"` sorts below `"aé"` as bytes, but `"a\u{FFFD}"` sorts above it. +/// +/// Spark's Filter above the scan re-evaluates every data filter, so a pushed filter only has to +/// keep every row the original keeps. That's why an `AND` keeps whichever side can be pushed. fn binary_as_string_pushdown_filter( filter: &Arc, required_schema: &Schema, filter_read_schema: &Schema, - parquet_options: &SparkParquetOptions, ) -> Result>, ExecutionError> { if !references_binary_read_column(filter, required_schema) { return Ok(Some(Arc::clone(filter))); } - let is_binary_column = |expr: &Arc| { + let reads_binary = |expr: &Arc| { expr.downcast_ref::() .is_some_and(|column| reads_binary(column, required_schema)) }; let pushdown = |expr: &Arc| { - binary_as_string_pushdown_filter(expr, required_schema, filter_read_schema, parquet_options) + binary_as_string_pushdown_filter(expr, required_schema, filter_read_schema) }; if let Some(binary) = filter.downcast_ref::() { @@ -162,13 +163,13 @@ fn binary_as_string_pushdown_filter( }); } Operator::Eq => { - if is_binary_column(left) { + if reads_binary(left) { if let Some(literal) = binary_literal(right) { let eq = BinaryExpr::new(Arc::clone(left), Operator::Eq, literal); return Ok(Some(Arc::new(eq))); } } - if is_binary_column(right) { + if reads_binary(right) { if let Some(literal) = binary_literal(left) { let eq = BinaryExpr::new(literal, Operator::Eq, Arc::clone(right)); return Ok(Some(Arc::new(eq))); @@ -178,72 +179,22 @@ fn binary_as_string_pushdown_filter( _ => {} } } else if let Some(list) = filter.downcast_ref::() { - if !list.negated() && is_binary_column(list.expr()) { + if !list.negated() && reads_binary(list.expr()) { if let Some(values) = list.list().iter().map(binary_literal).collect() { let expr = Arc::clone(list.expr()); return Ok(Some(in_list(expr, values, &false, filter_read_schema)?)); } } } else if let Some(is_null) = filter.downcast_ref::() { - if is_binary_column(is_null.arg()) { + if reads_binary(is_null.arg()) { return Ok(Some(Arc::clone(filter))); } } else if let Some(is_not_null) = filter.downcast_ref::() { - if is_binary_column(is_not_null.arg()) { + if reads_binary(is_not_null.arg()) { return Ok(Some(Arc::clone(filter))); } } - // Keep function evaluation above the scan. Parquet row filters flatten evaluation errors - // into Arrow errors, so moving a potentially throwing expression here changes its error. - let simple_comparison = filter.downcast_ref::().is_some_and(|binary| { - matches!( - binary.op(), - Operator::Eq - | Operator::NotEq - | Operator::Lt - | Operator::LtEq - | Operator::Gt - | Operator::GtEq - | Operator::IsDistinctFrom - | Operator::IsNotDistinctFrom - ) - }) || filter.downcast_ref::().is_some() - || filter.downcast_ref::().is_some(); - let simple_operands = filter.children().iter().all(|expr| { - expr.downcast_ref::().is_some() || expr.downcast_ref::().is_some() - }); - if !simple_comparison || !simple_operands { - return Ok(None); - } - // Keep other simple predicates over decoded strings. The per-file adapter can remove the decode - // for validated UTF8 columns, exposing ranges and LIKE to statistics pruning. Unannotated - // columns retain the decode, so row filters never compare malformed bytes as raw strings. - let decoded = Arc::clone(filter) - .transform(|expr| { - if expr - .downcast_ref::() - .is_some_and(|column| reads_binary(column, required_schema)) - { - Ok(Transformed::yes(binary_as_string_cast( - expr, - parquet_options, - ))) - } else { - Ok(Transformed::no(expr)) - } - }) - .data()?; - Ok(Some(decoded)) -} - -fn binary_as_string_cast( - child: Arc, - parquet_options: &SparkParquetOptions, -) -> Arc { - let mut options = SparkCastOptions::new(parquet_options.eval_mode, &parquet_options.timezone); - options.allow_cast_unsigned_ints = parquet_options.allow_cast_unsigned_ints; - options.is_adapting_schema = true; - Arc::new(Cast::new(child, DataType::Utf8, options, None, None)) + Ok(None) } /// `expr` as a Binary literal holding the same bytes, if it is a string literal without U+FFFD. @@ -274,7 +225,17 @@ fn binary_as_string_projection( let expr = if read_field.data_type() == output_field.data_type() { child } else { - binary_as_string_cast(child, parquet_options) + let mut cast_options = + SparkCastOptions::new(parquet_options.eval_mode, &parquet_options.timezone); + cast_options.allow_cast_unsigned_ints = parquet_options.allow_cast_unsigned_ints; + cast_options.is_adapting_schema = true; + Arc::new(Cast::new( + child, + output_field.data_type().clone(), + cast_options, + None, + None, + )) as Arc }; ProjectionExpr::new(expr, output_field.name()) })) @@ -506,13 +467,15 @@ pub(crate) fn init_datasource_exec( scan_io_source, parquet_source.metrics(), ) - .with_binary_as_string(binary_as_string) .with_spark_variant_schema(projects_variant) .with_require_field_ids(require_field_ids) .with_int96_leaf_stamp(rebase_from_file_metadata) .with_conversion_check( Arc::clone(&required_schema), - spark_parquet_options.clone(), + SparkParquetOptions { + reject_fixed_binary_as_string: binary_as_string, + ..spark_parquet_options.clone() + }, session_config.options().execution.parquet.pushdown_filters && data_filters .as_ref() @@ -542,7 +505,7 @@ pub(crate) fn init_datasource_exec( }); // In binaryAsString mode the filters are typed against the Utf8 output schema, but the scan // reads the string columns as Binary. Rewrite the filters on those columns that can compare - // raw bytes, and map the rest through the decoder. Filters on other columns are unchanged. + // raw bytes, and keep the rest out of the scan. Filters on other columns are pushed as is. let data_filters = match data_filters { Some(filters) if binary_as_string => { // The planner binds data filters against the required columns followed by the @@ -561,7 +524,6 @@ pub(crate) fn init_datasource_exec( &filter, &required_schema, &filter_read_schema, - &spark_parquet_options, )? { pushable.push(filter); } @@ -599,8 +561,7 @@ pub(crate) fn init_datasource_exec( file_source }; let expr_adapter_factory: Arc = Arc::new( - SparkPhysicalExprAdapterFactory::new(spark_parquet_options, default_values) - .with_binary_as_string(binary_as_string && !encryption_enabled), + SparkPhysicalExprAdapterFactory::new(spark_parquet_options, default_values), ); let file_groups = file_groups @@ -849,14 +810,6 @@ mod tests { file: &PartitionedFile, filters: Vec>, session_ctx: &Arc, - ) -> Arc { - binary_as_string_files_scan(std::slice::from_ref(file), filters, session_ctx) - } - - fn binary_as_string_files_scan( - files: &[PartitionedFile], - filters: Vec>, - session_ctx: &Arc, ) -> Arc { let required_schema = binary_as_string_required_schema(); init_datasource_exec( @@ -865,7 +818,7 @@ mod tests { None, ObjectStoreUrl::local_filesystem(), ObjectStoreBackend::Local, - vec![files.to_vec()], + vec![vec![file.clone()]], Some(vec![0, 1]), Some(filters), None, @@ -997,7 +950,8 @@ mod tests { } /// An unannotated BINARY column read as a string prunes on `=`, `IN` and `IS [NOT] NULL` - /// compared as raw bytes. Other predicates retain the decoder and only filter rows. + /// compared as raw bytes, while filters that could disagree with the decoded strings stay + /// above the scan. #[tokio::test] async fn binary_as_string_pushes_raw_byte_filters_on_unannotated_binary() { let file = write_binary_as_string_fixture( @@ -1020,10 +974,10 @@ mod tests { Arc::new(IsNotNullExpr::new(s())), groups(&[Some("one"), Some("\u{fffd}")]), ), - // Decoded ranges cannot prune raw-byte statistics. Row filtering still applies - // the range to the decoded strings and removes the null group. + // Pushed, this would prune the null group. It isn't, because a range on the decoded + // strings can disagree with the same range on the bytes. ("range", compare_s(Operator::Gt, "a"), every_group.clone()), - // NOT IN retains its decode and can filter rows, but cannot prune byte statistics. + // The scan pushes only `=`, `IN` and `IS [NOT] NULL`, so `NOT IN` stays above it. ("NOT IN", s_in(&["one"], true), every_group.clone()), // The invalid group decodes to U+FFFD, so comparing the literal's bytes would prune a // group that matches. @@ -1038,7 +992,7 @@ mod tests { s_in(&["one", "\u{fffd}"], false), every_group, ), - // A filter on another column still prunes inside an AND with a decoded predicate. + // A filter on another column still prunes, and `AND` keeps the side that is pushed. ( "AND", Arc::new(BinaryExpr::new( @@ -1053,17 +1007,7 @@ mod tests { for row_filter in [false, true] { assert_eq!( scan_binary_as_string(&file, vec![Arc::clone(&filter)], row_filter).await, - if row_filter { - match name { - "range" | "IN with a U+FFFD literal" => { - groups(&[Some("one"), Some("\u{fffd}")]) - } - "NOT IN" | "U+FFFD literal" => groups(&[Some("\u{fffd}")]), - _ => expected.clone(), - } - } else { - expected.clone() - }, + expected, "{name}, row filter {row_filter}" ); } @@ -1096,8 +1040,8 @@ mod tests { ); } - /// Annotated strings bypass the synthetic binary decode, keeping direct-column statistics - /// pruning for equality, ranges, LIKE, IN, NOT IN and null checks. + /// A column the file annotates as a string is read as Binary too, and the adapter's cast back + /// would hide it from pruning. The adapter compares the column directly instead. #[tokio::test] async fn binary_as_string_prunes_annotated_strings() { let file = write_binary_as_string_fixture( @@ -1108,21 +1052,10 @@ mod tests { let cases: Vec<(&str, Arc, DecodedCounts)> = vec![ ("=", compare_s(Operator::Eq, "one"), groups(&[Some("one")])), ("IN", s_in(&["two"], false), groups(&[Some("two")])), - ("NOT IN", s_in(&["one"], true), groups(&[Some("two")])), - ( - "range", - compare_s(Operator::Lt, "two"), - groups(&[Some("one")]), - ), ( - "LIKE", - Arc::new(datafusion::physical_expr::expressions::LikeExpr::new( - false, - false, - s(), - Arc::new(Literal::new(ScalarValue::Utf8(Some("on%".to_string())))), - )), - groups(&[Some("one")]), + "NOT IN", + s_in(&["one"], true), + groups(&[Some("one"), Some("two"), None]), ), ("IS NULL", Arc::new(IsNullExpr::new(s())), groups(&[None])), ( @@ -1142,50 +1075,6 @@ mod tests { } } - #[tokio::test] - async fn binary_as_string_keeps_throwing_functions_above_scan() { - let file = - write_binary_as_string_fixture(DataType::Binary, [Some(b"one"), Some(&[0xff]), None]); - let cast: Arc = Arc::new(Cast::new( - Arc::new(Column::new("s", 1)), - DataType::Int32, - SparkCastOptions::new(EvalMode::Ansi, "UTC"), - None, - None, - )); - let filter: Arc = Arc::new(IsNullExpr::new(cast)); - for row_filter in [false, true] { - assert_eq!( - scan_binary_as_string(&file, vec![filter.clone()], row_filter).await, - groups(&[Some("one"), Some("\u{fffd}"), None]) - ); - } - } - - #[tokio::test] - async fn binary_as_string_mixed_files_keep_unannotated_decode() { - let annotated = - write_binary_as_string_fixture(DataType::Utf8, [Some(b"a"), Some(b"zzz"), None]); - let binary = - write_binary_as_string_fixture(DataType::Binary, [Some(b"a\x80"), Some(b"a"), None]); - for row_filter in [false, true] { - let session = binary_as_string_session(row_filter); - let scan = binary_as_string_files_scan( - &[annotated.clone(), binary.clone()], - vec![compare_s(Operator::Lt, "aé")], - &session, - ); - let mut expected = BTreeMap::from([(Some("a".to_string()), 1_000)]); - if !row_filter { - // Only the annotated file prunes. Binary a\x80 must be decoded before its - // range is evaluated, since its raw and decoded ordering disagree. - expected.insert(Some("a\u{fffd}".to_string()), 500); - expected.insert(None, 500); - } - assert_eq!(decoded_counts(&scan, &session).await, expected); - } - } - #[tokio::test] async fn binary_as_string_rejects_fixed_binary_requested_as_string() { use futures::TryStreamExt; diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index e929ab6b106..a61541c0fe7 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -63,7 +63,6 @@ pub struct SparkPhysicalExprAdapterFactory { /// Default values for columns that may be missing from the physical schema. /// The key is the Column (containing name and index). default_values: Option>, - binary_as_string: bool, } impl SparkPhysicalExprAdapterFactory { @@ -75,15 +74,8 @@ impl SparkPhysicalExprAdapterFactory { Self { parquet_options, default_values, - binary_as_string: false, } } - - /// Elide the scan's synthetic decode only when the reader stamped the physical UTF8 field. - pub(crate) fn with_binary_as_string(mut self, enabled: bool) -> Self { - self.binary_as_string = enabled; - self - } } /// Returns true when casting `physical_type` to `target_type` is a *pure* structural @@ -1137,7 +1129,6 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { physical_file_schema: adapted_physical_schema, parquet_options: self.parquet_options.clone(), default_values: self.default_values.clone(), - binary_as_string: self.binary_as_string, default_adapter, logical_to_physical_names, original_physical_dup_check, @@ -1168,7 +1159,6 @@ struct SparkPhysicalExprAdapter { default_values: Option>, /// The default DataFusion adapter to delegate standard handling to default_adapter: Arc, - binary_as_string: bool, /// Mapping from logical column names to original physical column names, /// used for case-insensitive mode where names differ in casing. /// After the default adapter rewrites expressions using the remapped @@ -1229,8 +1219,6 @@ impl PhysicalExprAdapter for SparkPhysicalExprAdapter { // - CometCastColumnExpr (for Struct/List/Map, uses spark_parquet_convert) // - Spark Cast (for simple scalar types) rewritten - .transform(|e| self.unwrap_binary_as_string_cast(e)) - .data()? .transform(|e| self.unwrap_cast_to_binary_comparison(e)) .data()? .transform(|e| self.replace_with_spark_cast(e)) @@ -1457,36 +1445,6 @@ impl SparkPhysicalExprAdapter { .data() } - /// The reader identifies columns whose Parquet UTF8 annotation makes arrow-rs validate the - /// bytes. On those columns, decode(CAST(s AS Binary)) is identity. Run before replacing the - /// adapter's CastExpr so both projections and predicates can expose the original column. - fn unwrap_binary_as_string_cast( - &self, - expr: Arc, - ) -> DataFusionResult>> { - if self.binary_as_string { - if let Some(decode) = expr.downcast_ref::() { - if decode.data_type == DataType::Utf8 && decode.cast_options.is_adapting_schema { - if let Some((column, DataType::Utf8)) = - self.column_cast_to_binary(&decode.child) - { - let index = column.downcast_ref::().unwrap().index(); - if self - .physical_file_schema - .field(index) - .metadata() - .get(UTF8_VALIDATED_KEY) - .is_some_and(|value| value == "true") - { - return Ok(Transformed::yes(column)); - } - } - } - } - } - Ok(Transformed::no(expr)) - } - /// 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'..'` @@ -1812,9 +1770,6 @@ impl SparkPhysicalExprAdapter { } } -/// Private field hint derived from the raw footer, never trusted from an input file's Arrow hint. -pub(crate) const UTF8_VALIDATED_KEY: &str = "comet.parquet.utf8_validated"; - /// Private, in-memory hint used only while checking the raw footer's LIST encoding. pub(crate) const REPEATED_PRIMITIVE_KEY: &str = "comet.parquet.repeated_primitive"; @@ -1966,7 +1921,7 @@ pub(crate) mod test { use crate::parquet::parquet_support::SparkParquetOptions; use crate::parquet::schema_adapter::{ check_conversion, check_field_conversion, is_pure_structural_narrowing, ConversionCheck, - SparkPhysicalExprAdapterFactory, NANOS_TIMESTAMP_KEY, UTF8_VALIDATED_KEY, + SparkPhysicalExprAdapterFactory, NANOS_TIMESTAMP_KEY, }; use arrow::array::cast::AsArray; use arrow::array::UInt32Array; @@ -1993,7 +1948,7 @@ pub(crate) mod test { use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_plan::{ExecutionPlan, SendableRecordBatchStream}; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; - use datafusion_comet_spark_expr::{Cast, EvalMode, SparkCastOptions}; + use datafusion_comet_spark_expr::EvalMode; use datafusion_physical_expr_adapter::PhysicalExprAdapterFactory; use futures::StreamExt; use parquet::arrow::ArrowWriter; @@ -5744,57 +5699,6 @@ pub(crate) mod test { } } - #[test] - fn binary_as_string_decode_elision_requires_reader_validation() { - let logical = Arc::new(Schema::new(vec![Field::new("s", DataType::Binary, true)])); - for validated in [false, true] { - let metadata = if validated { - HashMap::from([(UTF8_VALIDATED_KEY.to_string(), "true".to_string())]) - } else { - HashMap::new() - }; - let physical = Arc::new(Schema::new(vec![ - Field::new("s", DataType::Utf8, true).with_metadata(metadata) - ])); - let adapter = SparkPhysicalExprAdapterFactory::new( - SparkParquetOptions::new(EvalMode::Legacy, "UTC"), - None, - ) - .with_binary_as_string(true) - .create(logical.clone(), physical) - .unwrap(); - let mut options = SparkCastOptions::new(EvalMode::Legacy, "UTC"); - options.is_adapting_schema = true; - let decode: Arc = Arc::new(Cast::new( - Arc::new(Column::new("s", 0)), - DataType::Utf8, - options, - None, - None, - )); - let rewritten = adapter.rewrite(decode).unwrap(); - assert_eq!(rewritten.downcast_ref::().is_some(), validated); - let range = Arc::new(BinaryExpr::new( - rewritten, - Operator::Lt, - Arc::new(Literal::new(ScalarValue::Utf8(Some("two".to_string())))), - )) as Arc; - let batch = RecordBatch::try_new( - Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, true)])), - vec![Arc::new(StringArray::from(vec![ - Some("one"), - Some("two"), - None, - ]))], - ) - .unwrap(); - let result = range.evaluate(&batch).unwrap().into_array(3).unwrap(); - assert_eq!( - result.as_boolean().iter().collect::>(), - vec![Some(true), Some(false), None] - ); - } - } #[test] fn binary_as_string_conversion_check_keeps_iceberg_uuid_adaptation() { let physical = Arc::new(Schema::new(vec![Field::new( diff --git a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala index 4bb1a45eebc..64d0402d53d 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -392,7 +392,7 @@ abstract class ParquetReadSuite extends CometTestBase { } // With binaryAsString, the native scan compares `=`, `IN` and `IS [NOT] NULL` on the raw bytes - // and evaluates other string filters on decoded values. Spark compares raw bytes, so these queries + // and keeps other string filters above the scan. Spark compares raw bytes too, so these queries // match it. A query that tells a malformed value apart from U+FFFD doesn't, as the // compatibility guide describes. test("spark.sql.parquet.binaryAsString filters, joins and top-k") { @@ -441,8 +441,6 @@ abstract class ParquetReadSuite extends CometTestBase { "SELECT * FROM t WHERE value > 'o'", "SELECT * FROM t WHERE value = 'one' OR id = 21", "SELECT * FROM annotated WHERE value = 'one'", - "SELECT * FROM annotated WHERE value < 'two'", - "SELECT * FROM annotated WHERE value LIKE 'on%'", "SELECT * FROM annotated WHERE value IS NULL").foreach { query => val (_, plan) = checkSparkAnswerAndOperator(sql(query)) assertNativeScan(plan) diff --git a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala index d63a3832ab7..a6f7bbe02d2 100644 --- a/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala +++ b/spark/src/test/scala/org/apache/spark/sql/benchmark/CometReadBenchmark.scala @@ -425,8 +425,9 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { /** * Spark-written strings carry the UTF8 annotation. With `spark.sql.parquet.binaryAsString`, the - * native scan's per-file adapter removes the synthetic binary decode for these columns. Compare - * full scans, equality, ranges and LIKE pruning with the setting off and on. + * native scan still reads them as binary and decodes them, and only `=`, `IN` and `IS [NOT] + * NULL` on them reach the reader. This compares a full scan and a selective filter with the + * setting off and on. */ def binaryAsStringScanBenchmark(values: Int): Unit = { withTempPath { dir => @@ -440,11 +441,7 @@ class CometReadBaseBenchmark extends CometBenchmarkBase { Seq( "Annotated String Scan" -> "SELECT sum(length(c1)) FROM parquetV1Table", "Annotated String Filter" -> - "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 = '500'", - "Annotated String Range" -> - "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 < '100'", - "Annotated String LIKE" -> - "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 LIKE '500%'").foreach { + "SELECT sum(length(c1)) FROM parquetV1Table WHERE c1 = '500'").foreach { case (name, query) => val benchmark = new Benchmark(name, values.toLong, output = output) Seq("false", "true").foreach { binaryAsString => From e8df434b587e660784eacffb40139c1643580539 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Tue, 10 Nov 2026 22:10:18 +0800 Subject: [PATCH 10/10] fix: use Arc::clone in the binaryAsString adapter test Co-Authored-By: Claude Fable 5.2 --- native/core/src/parquet/schema_adapter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index a61541c0fe7..368a852fde7 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -5711,7 +5711,7 @@ pub(crate) mod test { SparkParquetOptions::new(EvalMode::Legacy, "UTC"), None, ) - .create(logical, physical.clone()) + .create(logical, Arc::clone(&physical)) .unwrap(); let expr = adapter.rewrite(Arc::new(Column::new("s", 0))).unwrap(); let uuid = uuid::Uuid::from_bytes([42; 16]);