From 8b9343cf8048e10a05366e39c7f5e3de507ec0f7 Mon Sep 17 00:00:00 2001 From: Dustin Smith Date: Tue, 22 Sep 2026 22:46:57 +0700 Subject: [PATCH 1/4] fix: reject a file without field ids at any depth whether or not id matching is on Spark's `ParquetReadSupport.getRequestedSchema` raises when the requested schema carries a Parquet field id at any depth and the file carries none at any depth, unless `spark.sql.parquet.fieldId.read.ignoreMissing` is set. The check runs on every read, for both readers, and does not consult `spark.sql.parquet.fieldId.read.enabled`. The native scan ran that check only with the read flag on, only over root fields, and only inside the name remap, which a case-sensitive session with the flag off never reaches. A read schema with ids over a file without ids returned rows where Spark raises, and a file whose ids sit only on nested fields was rejected where Spark reads it and null-fills the unmatched fields. The check now runs at the top of the expression adapter factory with a recursive predicate on both schemas. Id matching itself stays gated on the read flag and root ids, as Spark gates it per struct level. The Iceberg scan opts out, since its reader resolves columns by id and supplies the ids itself. --- .../src/execution/operators/iceberg_scan.rs | 8 +- native/core/src/parquet/parquet_support.rs | 89 +++++++- native/core/src/parquet/schema_adapter.rs | 209 ++++++++++++++++-- .../comet/parquet/ParquetReadSuite.scala | 93 ++++++++ 4 files changed, 383 insertions(+), 16 deletions(-) diff --git a/native/core/src/execution/operators/iceberg_scan.rs b/native/core/src/execution/operators/iceberg_scan.rs index fdcd6c8ba5a..e6b0011757b 100644 --- a/native/core/src/execution/operators/iceberg_scan.rs +++ b/native/core/src/execution/operators/iceberg_scan.rs @@ -230,7 +230,13 @@ impl IcebergScanExec { let scan_metrics = scan_result.metrics().clone(); let stream = scan_result.stream(); - let spark_options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + let mut spark_options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + // Iceberg resolves columns by id itself and its reader supplies the ids, so the + // missing-id check that guards plain Parquet reads does not apply here. The same holds + // for a migrated table read through a name mapping: the reader still resolves the + // columns itself and hands back a schema of its own, so the Spark check has nothing to + // say there either. + spark_options.ignore_missing_field_id = true; let adapter_factory = SparkPhysicalExprAdapterFactory::new(spark_options, None); let adapted_stream = diff --git a/native/core/src/parquet/parquet_support.rs b/native/core/src/parquet/parquet_support.rs index d6ac9bed636..a9ee2ba6d0e 100644 --- a/native/core/src/parquet/parquet_support.rs +++ b/native/core/src/parquet/parquet_support.rs @@ -22,7 +22,7 @@ use arrow::array::{ }; use arrow::buffer::NullBuffer; use arrow::compute::can_cast_types; -use arrow::datatypes::{FieldRef, Fields}; +use arrow::datatypes::{Field, FieldRef, Fields}; use arrow::{ array::{ cast::AsArray, new_null_array, types::TimestampMicrosecondType, @@ -414,6 +414,39 @@ fn field_id(field: &arrow::datatypes::Field) -> Option { .and_then(|v| v.parse::().ok()) } +/// True when a field in `fields`, at any nesting depth, carries a Parquet field id. Spark's +/// `containsFieldIds` walks the whole file schema the same way, and `ParquetUtils.hasFieldIds` +/// walks the read schema. The root-only `schema_has_field_ids` in the schema adapter stays as the +/// gate for id matching, which only ever renames root fields. +pub(crate) fn any_nested_field_has_id(fields: &Fields) -> bool { + fields.iter().any(|f| field_holds_id(f)) +} + +/// Whether `field` or anything nested under it carries a Parquet field id. Dictionary and +/// run-end-encoded wrappers are not walked, because the Parquet read path never nests a struct, +/// list or map inside them. +fn field_holds_id(field: &Field) -> bool { + field_id(field).is_some() + || match field.data_type() { + DataType::Struct(fields) => any_nested_field_has_id(fields), + DataType::Map(entries, _) => field_holds_id(entries), + other => list_element_field(other).is_some_and(|f| field_holds_id(f)), + } +} + +/// The element field of a list in any Arrow representation, or `None` for a type that is not a +/// list. +fn list_element_field(data_type: &DataType) -> Option<&FieldRef> { + match data_type { + DataType::List(f) + | DataType::LargeList(f) + | DataType::FixedSizeList(f, _) + | DataType::ListView(f) + | DataType::LargeListView(f) => Some(f), + _ => None, + } +} + /// Resolve each requested (`to`) struct field to the index of the file (`from`) field it reads /// from, or `None` when the file holds no such field. Mirrors Spark's `clipParquetGroupFields`: /// when the requested struct carries Parquet field IDs anywhere (and `use_field_id` is set), @@ -2075,4 +2108,58 @@ mod tests { "unexpected error: {err}" ); } + + /// The recursive id check sees an id on a root field, on a struct child, on the element of + /// every list representation, and on a map key or value. It sees none on a schema without + /// ids and none on an empty schema. + #[test] + fn any_nested_field_has_id_finds_ids_at_every_depth() { + use super::any_nested_field_has_id; + use arrow::datatypes::{DataType, Field, Fields}; + use parquet::arrow::PARQUET_FIELD_ID_META_KEY; + + let plain = |name: &str| Field::new(name, DataType::Int32, true); + let tagged = |name: &str| { + plain(name).with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "7".to_string(), + )])) + }; + let root = |field: Field| Fields::from(vec![field]); + let has_id = |field: Field| any_nested_field_has_id(&root(field)); + + assert!(!any_nested_field_has_id(&Fields::empty())); + assert!(!has_id(plain("x"))); + assert!(has_id(tagged("x"))); + + let strukt = |child: Field| Field::new("s", DataType::Struct(root(child)), true); + assert!(has_id(strukt(tagged("c")))); + assert!(!has_id(strukt(plain("c")))); + + let element = Arc::new(tagged("item")); + for list_type in [ + DataType::List(Arc::clone(&element)), + DataType::LargeList(Arc::clone(&element)), + DataType::FixedSizeList(Arc::clone(&element), 2), + DataType::ListView(Arc::clone(&element)), + DataType::LargeListView(Arc::clone(&element)), + ] { + let list = Field::new("l", list_type.clone(), true); + assert!(has_id(list), "{list_type}"); + } + let plain_list = Field::new("l", DataType::List(Arc::new(plain("item"))), true); + assert!(!has_id(plain_list)); + + let map = |key: Field, value: Field| { + let entries = Field::new( + "entries", + DataType::Struct(Fields::from(vec![key, value])), + false, + ); + Field::new("m", DataType::Map(Arc::new(entries), false), true) + }; + assert!(has_id(map(tagged("key"), plain("value")))); + assert!(has_id(map(plain("key"), tagged("value")))); + assert!(!has_id(map(plain("key"), plain("value")))); + } } diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index e06408b44e1..769e3f7c652 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -18,7 +18,7 @@ use crate::parquet::cast_column::CometCastColumnExpr; use crate::parquet::name_fold::{fold_name, fold_names, fold_schema_names}; use crate::parquet::parquet_support::{ - match_struct_fields, spark_parquet_convert, SparkParquetOptions, + any_nested_field_has_id, match_struct_fields, spark_parquet_convert, SparkParquetOptions, }; use arrow::array::new_empty_array; use arrow::compute::can_cast_types; @@ -76,6 +76,10 @@ fn parse_field_id(field: &Field) -> Option { .and_then(|v| v.parse::().ok()) } +/// True when a root field of `schema` carries a Parquet field id. This stays root-only on +/// purpose: it gates the root name remap, and Spark's `clipParquetGroupFields` decides id +/// matching one struct level at a time. Whether a file holds ids at all is a different question, +/// answered at every depth by `any_nested_field_has_id` in `parquet_support`. fn schema_has_field_ids(schema: &SchemaRef) -> bool { schema.fields().iter().any(|f| parse_field_id(f).is_some()) } @@ -201,18 +205,11 @@ fn remap_physical_schema( physical_schema: &SchemaRef, case_sensitive: bool, use_field_id: bool, - ignore_missing_field_id: bool, ) -> DataFusionResult<(SchemaRef, HashMap)> { + // Root ids alone decide whether to match by id here. The check that the file holds ids at + // all runs earlier, in `create`, and looks at every nesting level. let should_match_by_id = use_field_id && schema_has_field_ids(logical_schema); - if should_match_by_id && !ignore_missing_field_id && !schema_has_field_ids(physical_schema) { - // Mirrors `ParquetReadSupport.inferSchema`'s eager check (Spark throws a runtime - // error rather than silently returning null columns). - return Err(DataFusionError::External(Box::new( - SparkError::ParquetMissingFieldIds, - ))); - } - // Build id -> all matching physical field names. We need the full list so we can mirror // Spark's `_LEGACY_ERROR_TEMP_2094` "Found duplicate field(s)" error when an ID-bearing // logical field would resolve to more than one physical field. @@ -852,6 +849,21 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { // to the original physical names. This is necessary because downstream code // (reassign_expr_columns) looks up columns by name in the actual stream schema, // which uses the original physical file column names. + // + // Before any of that, mirror the eager check in Spark's `ParquetReadSupport`: a read + // schema that carries field ids at any depth may not read a file that carries none at + // any depth, unless `ignoreMissing` is set. Spark applies this check whether or not + // `fieldId.read.enabled` is on, so it runs before the id matching gate below and does + // not depend on the remap. + if !self.parquet_options.ignore_missing_field_id + && any_nested_field_has_id(logical_file_schema.fields()) + && !any_nested_field_has_id(physical_file_schema.fields()) + { + return Err(DataFusionError::External(Box::new( + SparkError::ParquetMissingFieldIds, + ))); + } + let case_sensitive = self.parquet_options.case_sensitive; let should_match_by_id = self.parquet_options.use_field_id && schema_has_field_ids(&logical_file_schema); @@ -863,7 +875,6 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { &physical_file_schema, case_sensitive, self.parquet_options.use_field_id, - self.parquet_options.ignore_missing_field_id, )?; // Build the folded-name -> original-physical-field-indices map once for per-column // duplicate detection, paired with the original schema so the rare error path can @@ -2859,6 +2870,176 @@ mod test { Ok(()) } + /// Run `scan_parquet` and return the message of the error it raises, either while planning + /// the scan or on the first poll of the stream. + async fn scan_error_message( + batch: &RecordBatch, + required_schema: SchemaRef, + options: SparkParquetOptions, + ) -> String { + match scan_parquet(batch, required_schema, options) { + Err(err) => err.to_string(), + Ok(mut stream) => stream + .next() + .await + .unwrap() + .expect_err("expected the scan to be rejected") + .to_string(), + } + } + + /// Batch `s: struct` with no id on the root field, so a file written from it + /// carries ids only below the root. + fn nested_id_batch() -> Result { + struct_batch( + Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")), + Arc::new(Int32Array::from(vec![1, 2])), + ) + } + + /// Spark checks for missing file ids before it decides whether to match by id, so the + /// rejection fires even when `fieldId.read.enabled` is off. A case-sensitive session keeps + /// the name remap out of the picture. + #[tokio::test] + async fn missing_file_field_ids_rejected_when_id_matching_disabled() { + let file_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); + let batch = RecordBatch::try_new( + file_schema, + vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], + ) + .unwrap(); + let required_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) + ])); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = false; + options.case_sensitive = true; + let msg = scan_error_message(&batch, required_schema, options).await; + assert!( + msg.contains("Parquet file schema doesn't contain any field Ids"), + "unexpected error: {msg}" + ); + } + + /// A read schema whose only ids sit on struct children still expects ids, as Spark's + /// `ParquetUtils.hasFieldIds` walks nested fields. A file with no ids anywhere is rejected. + #[tokio::test] + async fn nested_logical_field_ids_rejected_when_file_has_none() -> Result<(), DataFusionError> { + let batch = struct_batch( + Field::new("a", DataType::Int32, true), + Arc::new(Int32Array::from(vec![1, 2])), + )?; + let required_schema = struct_schema(vec![ + Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")) + ]); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = true; + let msg = scan_error_message(&batch, required_schema, options).await; + assert!( + msg.contains("Parquet file schema doesn't contain any field Ids"), + "unexpected error: {msg}" + ); + Ok(()) + } + + /// Ids that only appear below the root of the file still count as ids, as in Spark's + /// `containsFieldIds`. The read passes the missing-id check, resolves the nested field by + /// id, and null-fills a root field whose id the file does not hold. + #[tokio::test] + async fn nested_file_field_ids_satisfy_the_missing_id_check() -> Result<(), DataFusionError> { + let batch = nested_id_batch()?; + let required_schema = Arc::new(Schema::new(vec![ + Field::new( + "s", + DataType::Struct(Fields::from(vec![ + Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")) + ])), + true, + ), + Field::new("missing", DataType::Int32, true).with_metadata(id_meta("7")), + ])); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = true; + let mut stream = scan_parquet(&batch, required_schema, options)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.num_rows(), 2); + let s = result.column(0).as_struct(); + assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); + assert_eq!(result.column(1).null_count(), 2); + Ok(()) + } + + /// With `ignoreMissing` set, a file without ids reads under an id-bearing schema and the + /// unmatched field is null-filled instead of raising. + #[tokio::test] + async fn missing_file_field_ids_allowed_when_ignore_missing_is_set( + ) -> Result<(), DataFusionError> { + let file_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); + let batch = RecordBatch::try_new( + file_schema, + vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], + )?; + let required_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) + ])); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = true; + options.ignore_missing_field_id = true; + let mut stream = scan_parquet(&batch, required_schema, options)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.num_rows(), 2); + assert_eq!(result.column(0).null_count(), 2); + Ok(()) + } + + /// With id matching off, ids on both sides play no part: the file is not rejected and the + /// read schema field still resolves by name. A name the file lacks reads as null even though + /// the file holds a field with the same id. + #[tokio::test] + async fn file_ids_ignored_when_id_matching_disabled() -> Result<(), DataFusionError> { + let file_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) + ])); + let batch = RecordBatch::try_new( + file_schema, + vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], + )?; + let required_schema = Arc::new(Schema::new(vec![ + Field::new("b", DataType::Int32, true).with_metadata(id_meta("1")) + ])); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = false; + options.ignore_missing_field_id = false; + let mut stream = scan_parquet(&batch, required_schema, options)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.num_rows(), 2); + assert_eq!(result.column(0).null_count(), 2); + Ok(()) + } + + /// A read schema without ids never triggers the missing-id check, whatever the flags say. + /// The file name differs only in case so the adapter is still created. + #[tokio::test] + async fn no_logical_field_ids_never_rejects() -> Result<(), DataFusionError> { + let file_schema = Arc::new(Schema::new(vec![Field::new("A", DataType::Int32, true)])); + let batch = RecordBatch::try_new( + file_schema, + vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], + )?; + let required_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); + let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); + options.use_field_id = true; + options.ignore_missing_field_id = false; + options.case_sensitive = false; + let mut stream = scan_parquet(&batch, required_schema, options)?; + let result = stream.next().await.unwrap()?; + assert_eq!( + result.column(0).as_primitive::().values(), + &[1, 2] + ); + Ok(()) + } + /// Disallowed widening (`INT32 -> bigint` with `allow_type_promotion` off) inside a /// struct defers to `RejectOnNonEmpty`, like the top level: a non-empty file fails ... #[tokio::test] @@ -3106,7 +3287,7 @@ mod test { let logical = Arc::new(Schema::new(vec![Field::new("Name", DataType::Int32, true)])); let physical = Arc::new(Schema::new(vec![Field::new("NAME", DataType::Int32, true)])); let (remapped, name_map) = - super::remap_physical_schema(&logical, &physical, false, false, false).unwrap(); + super::remap_physical_schema(&logical, &physical, false, false).unwrap(); assert_eq!(remapped.field(0).name(), "Name"); assert_eq!(name_map.get("Name").map(String::as_str), Some("NAME")); } @@ -3123,7 +3304,7 @@ mod test { ])); let physical = Arc::new(Schema::new(vec![Field::new("FOO", DataType::Int32, true)])); let (remapped, _name_map) = - super::remap_physical_schema(&logical, &physical, false, true, true).unwrap(); + super::remap_physical_schema(&logical, &physical, false, true).unwrap(); assert!( remapped .field(0) @@ -3151,7 +3332,7 @@ mod test { Field::new("a", DataType::Int32, true).with_metadata(id_meta("9")) ])); let (remapped, _name_map) = - super::remap_physical_schema(&logical, &physical, true, true, false).unwrap(); + super::remap_physical_schema(&logical, &physical, true, true).unwrap(); assert_eq!(remapped.field(0).name(), "a"); } 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 d412918b572..b4e4ce68b1e 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -2152,6 +2152,99 @@ abstract class ParquetReadSuite extends CometTestBase { } } } + + // Spark's `ParquetReadSupport` checks for missing file ids before it looks at + // `fieldId.read.enabled`, so the error is raised with id matching off as well. With + // `ignoreMissing` set, both engines fall back to matching by name and read real values. + test("read schema with field ids raises on a file without ids when id matching is off") { + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { + withTempPath { dir => + val readSchema = new StructType().add("a", IntegerType, true, withId(1)) + val writeSchema = new StructType().add("a", IntegerType, true) + val writeData = Seq(Row(100), Row(200)) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + def readCause(): Throwable = intercept[SparkException] { + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() + }.getCause + def assertMissingIds(cause: Throwable): Unit = { + assert( + cause.isInstanceOf[RuntimeException] && + cause.getMessage.contains("Parquet file schema doesn't contain any field Ids"), + cause) + } + + withClue("Spark with Comet disabled") { + withSQLConf(CometConf.COMET_ENABLED.key -> "false") { + assertMissingIds(readCause()) + } + } + withClue("Comet") { + assertMissingIds(readCause()) + } + + withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { + checkSparkAnswerAndOperator(spark.read.schema(readSchema).parquet(dir.getCanonicalPath)) + } + } + } + } + + // Spark's `containsFieldIds` walks the whole file schema, so ids that sit only on struct + // children count. The root field whose id the file lacks is null filled rather than rejected. + test("a file whose field ids are only on nested fields reads without a missing-id error") { + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "true") { + withTempPath { dir => + val nested = StructType(Seq(StructField("a", IntegerType, nullable = true, withId(11)))) + val writeSchema = new StructType().add("s", nested, true) + val readSchema = new StructType() + .add("s", nested, true) + .add("missing", IntegerType, true, withId(7)) + val writeData = Seq(Row(Row(1)), Row(Row(2))) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + checkSparkAnswerAndOperator(spark.read.schema(readSchema).parquet(dir.getCanonicalPath)) + } + } + } + + // Second half of Spark `ParquetFieldIdIOSuite.test("global read/write flag should work + // correctly")`: the file carries ids but the read flag is off, so columns resolve by name + // only. None of the read names exist in the file, so every value is null and nothing raises. + test("field ids in the file are ignored when id matching is off") { + withSQLConf( + SQLConf.PARQUET_FIELD_ID_WRITE_ENABLED.key -> "true", + SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { + withTempPath { dir => + val readSchema = new StructType() + .add("some", IntegerType, true, withId(1)) + .add("other", StringType, true, withId(2)) + .add("name", StringType, true, withId(3)) + val writeSchema = new StructType() + .add("a", IntegerType, true, withId(1)) + .add("rand1", StringType, true, withId(2)) + .add("rand2", StringType, true, withId(3)) + val writeData = Seq(Row(100, "text", "txt"), Row(200, "more", "mr")) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + val df = spark.read.schema(readSchema).parquet(dir.getCanonicalPath) + checkSparkAnswerAndOperator(df) + checkAnswer(df, Row(null, null, null) :: Row(null, null, null) :: Nil) + } + } + } } class ParquetReadV1Suite extends ParquetReadSuite with AdaptiveSparkPlanHelper { From ba0d8d7c2d8a11b06dae6e6e75ef217561777a33 Mon Sep 17 00:00:00 2001 From: Dustin Smith Date: Wed, 23 Sep 2026 08:34:22 +0700 Subject: [PATCH 2/4] fix: answer both halves of the missing-field-id check from what Spark reads Spark checks the pruned required schema for ids and the raw Parquet schema for their absence. The requested half now comes from `required_schema` at plan time, since the logical schema DataFusion hands the adapter is the full read schema, so an id on a column the query never projects no longer raises. The file half moves into the eager page index reader, which walks the raw schema from the footer. The Arrow schema the adapter sees has lost container metadata after INT96 coercion and never carries ids on repeated list or key value groups, so a file whose only id sat on a struct holding a timestamp raised where Spark reads. The JNI error conversion unwraps the Parquet external error so the Java side keeps the same exception, and the Iceberg scan no longer needs an opt-out since it does not use that reader. Tests cover the pruned projection, the timestamp struct, an id only on a list or key value group, a directory mixing files with and without ids, nested schema pruning, id zero, count(*), and the exception class on each version. --- .../src/execution/operators/iceberg_scan.rs | 8 +- .../eager_page_index_reader_factory.rs | 125 +++- native/core/src/parquet/parquet_exec.rs | 13 +- native/core/src/parquet/parquet_support.rs | 29 +- native/core/src/parquet/schema_adapter.rs | 585 +++++++++++++++--- native/jni-bridge/src/errors.rs | 50 +- .../comet/parquet/ParquetReadSuite.scala | 215 ++++++- 7 files changed, 916 insertions(+), 109 deletions(-) diff --git a/native/core/src/execution/operators/iceberg_scan.rs b/native/core/src/execution/operators/iceberg_scan.rs index e6b0011757b..fdcd6c8ba5a 100644 --- a/native/core/src/execution/operators/iceberg_scan.rs +++ b/native/core/src/execution/operators/iceberg_scan.rs @@ -230,13 +230,7 @@ impl IcebergScanExec { let scan_metrics = scan_result.metrics().clone(); let stream = scan_result.stream(); - let mut spark_options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - // Iceberg resolves columns by id itself and its reader supplies the ids, so the - // missing-id check that guards plain Parquet reads does not apply here. The same holds - // for a migrated table read through a name mapping: the reader still resolves the - // columns itself and hands back a schema of its own, so the Spark check has nothing to - // say there either. - spark_options.ignore_missing_field_id = true; + let spark_options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); let adapter_factory = SparkPhysicalExprAdapterFactory::new(spark_options, None); let adapted_stream = 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 d89a1772835..0538ef48383 100644 --- a/native/core/src/parquet/eager_page_index_reader_factory.rs +++ b/native/core/src/parquet/eager_page_index_reader_factory.rs @@ -45,6 +45,13 @@ //! //! Filed upstream as apache/datafusion#23978. Revert this once the opener merges its deferred //! page-index load back into `FileMetadataCache` instead of bypassing it. +//! +//! The reader also carries Spark's missing field id check, because the footer is first at hand +//! here. Spark's `ParquetReadSupport` refuses to open a file whose raw schema carries no field +//! id when the requested schema carries one, unless `ignoreMissing` is set, and it walks the +//! raw `MessageType` to decide. The Arrow schema the schema adapter sees later cannot stand in +//! for that walk: the INT96 coercion rebuilds container fields without their metadata, and an +//! id on a `list` or `key_value` group, or on the message root, never reaches an Arrow field. use arrow::datatypes::{DataType, FieldRef, Schema}; use async_trait::async_trait; @@ -58,6 +65,7 @@ use datafusion::execution::cache::cache_manager::FileMetadataCache; use datafusion::physical_plan::metrics::{ Count, ExecutionPlanMetricsSet, MetricBuilder, MetricCategory, MetricType, }; +use datafusion_comet_common::SparkError; use datafusion_datasource::PartitionedFile; use futures::future::BoxFuture; use futures::{FutureExt, StreamExt, TryStreamExt}; @@ -77,7 +85,7 @@ use parquet::file::metadata::{FileMetaData, KeyValue, ParquetMetaDataBuilder}; use parquet::file::metadata::{ FooterTail, PageIndexPolicy, ParquetMetaData, ParquetMetaDataReader, }; -use parquet::schema::types::{ColumnDescPtr, SchemaDescriptor}; +use parquet::schema::types::{ColumnDescPtr, SchemaDescriptor, Type as ParquetType}; use std::fmt::{Debug, Display, Formatter}; use std::ops::Range; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; @@ -163,6 +171,11 @@ 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, + // Whether the schema Spark asked the scan for carries a field id at any depth, computed + // once by the planner. Together with `ignore_missing_field_id` it decides whether a file + // whose Parquet schema carries no id is refused, as Spark's `ParquetReadSupport` does. + requested_schema_has_field_ids: bool, + ignore_missing_field_id: bool, } impl EagerPageIndexReaderFactory { @@ -191,6 +204,8 @@ impl EagerPageIndexReaderFactory { metadata_cache, scan_io_metrics, spark_variant_schema: false, + requested_schema_has_field_ids: false, + ignore_missing_field_id: false, } } @@ -198,6 +213,31 @@ impl EagerPageIndexReaderFactory { self.spark_variant_schema = enabled; self } + + /// Arm Spark's missing field id check. A file whose Parquet schema carries no field id + /// is refused on open when `requested_schema_has_field_ids` is set and + /// `ignore_missing_field_id` is not. Both default to off, so a factory that never calls + /// this reads every file. + pub fn with_missing_field_id_check( + mut self, + requested_schema_has_field_ids: bool, + ignore_missing_field_id: bool, + ) -> Self { + self.requested_schema_has_field_ids = requested_schema_has_field_ids; + self.ignore_missing_field_id = ignore_missing_field_id; + self + } +} + +/// True when `node` or any node under it carries a field id, the way Spark's +/// `containsFieldIds` answers it over the raw Parquet schema, message root included. +fn parquet_schema_has_field_ids(node: &ParquetType) -> bool { + node.get_basic_info().has_id() + || (node.is_group() + && node + .get_fields() + .iter() + .any(|field| parquet_schema_has_field_ids(field))) } impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { @@ -225,6 +265,8 @@ impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { metadata_cache: Arc::clone(&self.metadata_cache), metadata_size_hint, spark_variant_schema: self.spark_variant_schema, + requested_schema_has_field_ids: self.requested_schema_has_field_ids, + ignore_missing_field_id: self.ignore_missing_field_id, })) } } @@ -240,6 +282,8 @@ struct EagerPageIndexReader { metadata_cache: Arc, metadata_size_hint: Option, spark_variant_schema: bool, + requested_schema_has_field_ids: bool, + ignore_missing_field_id: bool, } // Arrow infers ENUM as Binary, losing the distinction from raw binary that Spark needs. @@ -439,6 +483,8 @@ impl AsyncFileReader for EagerPageIndexReader { let metadata_size_hint = self.metadata_size_hint; let scan_io_metrics = Arc::clone(&self.scan_io_metrics); let spark_variant_schema = self.spark_variant_schema; + let require_file_field_ids = + self.requested_schema_has_field_ids && !self.ignore_missing_field_id; async move { let file_decryption_properties = options .and_then(|o| o.file_decryption_properties()) @@ -498,6 +544,20 @@ impl AsyncFileReader for EagerPageIndexReader { } let metadata = metadata?; + // Spark's `ParquetReadSupport` refuses to open a file that carries no field ids when + // the requested schema carries some, unless `ignoreMissing` is set, and it walks the + // raw `MessageType` to decide. The same walk runs here over the footer's schema. The + // error keeps its Spark type through `ParquetError::External`, which the JNI layer + // unwraps, so the JVM sees the same exception Spark raises. + if require_file_field_ids + && !parquet_schema_has_field_ids( + metadata.file_metadata().schema_descr().root_schema(), + ) + { + return Err(ParquetError::External(Box::new( + SparkError::ParquetMissingFieldIds, + ))); + } if spark_variant_schema { with_spark_arrow_schema(metadata) } else { @@ -992,6 +1052,69 @@ mod tests { ); } + /// The raw schema walk answers like Spark's `containsFieldIds`: an id on the message root, + /// on a leaf, or on a repeated `list` group counts, and a schema without any does not. + #[test] + fn parquet_schema_has_field_ids_sees_ids_on_any_node() { + use parquet::basic::Type as PhysicalType; + use parquet::schema::types::TypePtr; + + let leaf = |name: &str, id: Option| -> TypePtr { + Arc::new( + ParquetType::primitive_type_builder(name, PhysicalType::INT32) + .with_id(id) + .build() + .unwrap(), + ) + }; + let group = |name: &str, id: Option, fields: Vec| -> TypePtr { + Arc::new( + ParquetType::group_type_builder(name) + .with_id(id) + .with_fields(fields) + .build() + .unwrap(), + ) + }; + + assert!(!parquet_schema_has_field_ids(&group( + "schema", + None, + vec![] + ))); + assert!(!parquet_schema_has_field_ids(&group( + "schema", + None, + vec![leaf("a", None)] + ))); + assert!(parquet_schema_has_field_ids(&group( + "schema", + Some(1), + vec![leaf("a", None)] + ))); + assert!(parquet_schema_has_field_ids(&group( + "schema", + None, + vec![leaf("a", Some(1))] + ))); + let list_group_only = group( + "schema", + None, + vec![group( + "l", + None, + vec![group("list", Some(5), vec![leaf("element", None)])], + )], + ); + assert!(parquet_schema_has_field_ids(&list_group_only)); + let nested_without_ids = group( + "schema", + None, + vec![group("s", None, vec![leaf("a", None)])], + ); + assert!(!parquet_schema_has_field_ids(&nested_without_ids)); + } + #[test] fn variant_policy_preserves_footer_metadata_and_indexes() { let schema = Arc::new(Schema::new_with_metadata( diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index 5d70f2aaa76..48eaae1a947 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -20,7 +20,7 @@ use crate::parquet::eager_page_index_reader_factory::{EagerPageIndexReaderFactor use crate::parquet::encryption_support::{CometEncryptionConfig, ENCRYPTION_FACTORY_ID}; use crate::parquet::name_fold::fold_schema_names; use crate::parquet::parquet_support::{ - object_store_authority, ObjectStoreBackend, SparkParquetOptions, + any_nested_field_has_id, object_store_authority, ObjectStoreBackend, SparkParquetOptions, }; use crate::parquet::schema_adapter::SparkPhysicalExprAdapterFactory; use arrow::datatypes::{Field, FieldRef, SchemaRef}; @@ -102,6 +102,11 @@ pub(crate) fn init_datasource_exec( ); spark_parquet_options.use_field_id = use_field_id; spark_parquet_options.ignore_missing_field_id = ignore_missing_field_id; + // Spark runs its missing-id check against the pruned read schema it hands the reader, not + // the full data schema that DataFusion later passes the schema adapter, so the answer is + // taken from `required_schema` here, once per scan, and handed to the reader factory below. + spark_parquet_options.requested_schema_has_field_ids = + any_nested_field_has_id(required_schema.fields()); // Spark can discard filtered-out values before timestamp conversion using statistics, // dictionary, and row-level filters. Comet cannot mirror every pruning path, so applying // checked conversion in a filtered scan can fail on values Spark never reads. Preserve the @@ -194,7 +199,11 @@ pub(crate) fn init_datasource_exec( scan_io_source, parquet_source.metrics(), ) - .with_spark_variant_schema(projects_variant), + .with_spark_variant_schema(projects_variant) + .with_missing_field_id_check( + spark_parquet_options.requested_schema_has_field_ids, + spark_parquet_options.ignore_missing_field_id, + ), ); parquet_source = parquet_source.with_parquet_file_reader_factory(reader_factory); diff --git a/native/core/src/parquet/parquet_support.rs b/native/core/src/parquet/parquet_support.rs index a9ee2ba6d0e..6dae6673e68 100644 --- a/native/core/src/parquet/parquet_support.rs +++ b/native/core/src/parquet/parquet_support.rs @@ -104,6 +104,11 @@ pub struct SparkParquetOptions { /// requested schema does carry ids raises a runtime error rather than silently /// producing nulls (mirrors `spark.sql.parquet.fieldId.read.ignoreMissing`). pub ignore_missing_field_id: bool, + /// Whether the schema Spark asked the scan for carries a Parquet field id at any depth. + /// Spark's `ParquetReadSupport` runs its missing-id check against that pruned schema, so + /// the planner computes this once from `required_schema`. The reader factory tests it when + /// it reads a file's footer, against the Parquet schema found there. + pub requested_schema_has_field_ids: bool, /// Whether type promotion (schema evolution) is allowed, e.g. INT32 -> INT64, /// FLOAT -> DOUBLE. Mirrors spark.comet.schemaEvolution.enabled. pub allow_type_promotion: bool, @@ -132,6 +137,7 @@ impl SparkParquetOptions { return_null_struct_if_all_fields_missing: true, use_field_id: false, ignore_missing_field_id: false, + requested_schema_has_field_ids: false, allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, @@ -149,6 +155,7 @@ impl SparkParquetOptions { return_null_struct_if_all_fields_missing: true, use_field_id: false, ignore_missing_field_id: false, + requested_schema_has_field_ids: false, allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, @@ -416,15 +423,27 @@ fn field_id(field: &arrow::datatypes::Field) -> Option { /// True when a field in `fields`, at any nesting depth, carries a Parquet field id. Spark's /// `containsFieldIds` walks the whole file schema the same way, and `ParquetUtils.hasFieldIds` -/// walks the read schema. The root-only `schema_has_field_ids` in the schema adapter stays as the -/// gate for id matching, which only ever renames root fields. +/// walks the read schema. The planner runs this over the requested schema once, at plan time. +/// The file side is not an Arrow walk at all: the reader factory checks the Parquet schema in +/// the footer, because the Arrow schema the adapter sees can lose ids (the INT96 coercion +/// rebuilds container fields without their metadata) and never shows an id that sits on a +/// `list` or `key_value` group. The root-only `schema_has_field_ids` in the schema adapter +/// stays as the gate for id matching, which only ever renames root fields. pub(crate) fn any_nested_field_has_id(fields: &Fields) -> bool { fields.iter().any(|f| field_holds_id(f)) } -/// Whether `field` or anything nested under it carries a Parquet field id. Dictionary and -/// run-end-encoded wrappers are not walked, because the Parquet read path never nests a struct, -/// list or map inside them. +/// Whether `field` or anything nested under it carries a Parquet field id. +/// +/// This walks the requested schema, where only struct fields can hold the metadata: Spark's +/// `hasFieldIds` recurses through `ArrayType` and `MapType` into their element and key or value +/// types, only a `StructField` carries metadata, and the serde never populates the element or +/// key and value fields. The walk still descends through list and map fields to reach the +/// structs nested inside them. The file side is checked by the reader factory over the raw +/// Parquet schema, where any node can carry an id, as Spark's `containsFieldIds` does. +/// +/// Dictionary and run-end-encoded wrappers are not walked, because the Parquet read path never +/// nests a struct, list or map inside them. fn field_holds_id(field: &Field) -> bool { field_id(field).is_some() || match field.data_type() { diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index 769e3f7c652..4123f559024 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -18,7 +18,7 @@ use crate::parquet::cast_column::CometCastColumnExpr; use crate::parquet::name_fold::{fold_name, fold_names, fold_schema_names}; use crate::parquet::parquet_support::{ - any_nested_field_has_id, match_struct_fields, spark_parquet_convert, SparkParquetOptions, + match_struct_fields, spark_parquet_convert, SparkParquetOptions, }; use arrow::array::new_empty_array; use arrow::compute::can_cast_types; @@ -79,7 +79,7 @@ fn parse_field_id(field: &Field) -> Option { /// True when a root field of `schema` carries a Parquet field id. This stays root-only on /// purpose: it gates the root name remap, and Spark's `clipParquetGroupFields` decides id /// matching one struct level at a time. Whether a file holds ids at all is a different question, -/// answered at every depth by `any_nested_field_has_id` in `parquet_support`. +/// answered by the reader factory over the raw Parquet schema before the adapter is built. fn schema_has_field_ids(schema: &SchemaRef) -> bool { schema.fields().iter().any(|f| parse_field_id(f).is_some()) } @@ -207,7 +207,7 @@ fn remap_physical_schema( use_field_id: bool, ) -> DataFusionResult<(SchemaRef, HashMap)> { // Root ids alone decide whether to match by id here. The check that the file holds ids at - // all runs earlier, in `create`, and looks at every nesting level. + // all runs earlier, in the reader factory, over the raw Parquet schema. let should_match_by_id = use_field_id && schema_has_field_ids(logical_schema); // Build id -> all matching physical field names. We need the full list so we can mirror @@ -850,20 +850,11 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { // (reassign_expr_columns) looks up columns by name in the actual stream schema, // which uses the original physical file column names. // - // Before any of that, mirror the eager check in Spark's `ParquetReadSupport`: a read - // schema that carries field ids at any depth may not read a file that carries none at - // any depth, unless `ignoreMissing` is set. Spark applies this check whether or not - // `fieldId.read.enabled` is on, so it runs before the id matching gate below and does - // not depend on the remap. - if !self.parquet_options.ignore_missing_field_id - && any_nested_field_has_id(logical_file_schema.fields()) - && !any_nested_field_has_id(physical_file_schema.fields()) - { - return Err(DataFusionError::External(Box::new( - SparkError::ParquetMissingFieldIds, - ))); - } - + // The check that a file carries field ids at all, which Spark's `ParquetReadSupport` + // runs before anything else, lives in `EagerPageIndexReader::get_metadata`, where the + // raw Parquet schema is at hand. By the time the schemas reach this point the INT96 + // coercion may have dropped the ids from container fields, so `physical_file_schema` + // cannot answer that question. let case_sensitive = self.parquet_options.case_sensitive; let should_match_by_id = self.parquet_options.use_field_id && schema_has_field_ids(&logical_file_schema); @@ -1544,7 +1535,8 @@ impl PhysicalExpr for RejectOnNonEmpty { #[cfg(test)] mod test { use crate::parquet::cast_column::CometCastColumnExpr; - use crate::parquet::parquet_support::SparkParquetOptions; + use crate::parquet::parquet_exec::init_datasource_exec; + use crate::parquet::parquet_support::{ObjectStoreBackend, SparkParquetOptions}; use crate::parquet::schema_adapter::{ check_conversion, is_pure_structural_narrowing, ConversionCheck, SparkPhysicalExprAdapterFactory, @@ -1572,12 +1564,18 @@ mod test { use datafusion::physical_expr::expressions::Column; use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_plan::{ExecutionPlan, SendableRecordBatchStream}; + use datafusion::prelude::SessionContext; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; use datafusion_comet_spark_expr::EvalMode; use datafusion_physical_expr_adapter::PhysicalExprAdapterFactory; use futures::StreamExt; + use parquet::arrow::arrow_writer::ArrowWriterOptions; use parquet::arrow::ArrowWriter; use parquet::arrow::PARQUET_FIELD_ID_META_KEY; + use parquet::basic::{LogicalType, Repetition, Type as PhysicalType}; + use parquet::data_type::{Int32Type as ParquetInt32Type, Int96, Int96Type}; + use parquet::file::writer::SerializedFileWriter; + use parquet::schema::types::Type as ParquetType; use parquet::variant::VariantType; use std::collections::HashMap; use std::fs::File; @@ -2870,14 +2868,105 @@ mod test { Ok(()) } - /// Run `scan_parquet` and return the message of the error it raises, either while planning - /// the scan or on the first poll of the stream. - async fn scan_error_message( + /// The message every rejected read carries, from `SparkError::ParquetMissingFieldIds`. + const MISSING_IDS: &str = "Parquet file schema doesn't contain any field Ids"; + + /// A fresh temporary file path as a `String`. + fn temp_filename() -> String { + let filename = get_temp_filename(); + filename.as_path().as_os_str().to_str().unwrap().to_string() + } + + /// Write `batch` to a temporary file and return its path. With `skip_arrow_metadata` the + /// file carries no Arrow schema hint, so the reader derives every field, ids included, from + /// the Parquet schema alone. + fn write_parquet( batch: &RecordBatch, + skip_arrow_metadata: bool, + ) -> Result { + let filename = temp_filename(); + let file = File::create(&filename)?; + let options = ArrowWriterOptions::new().with_skip_arrow_metadata(skip_arrow_metadata); + let mut writer = ArrowWriter::try_new_with_options(file, batch.schema(), options)?; + writer.write(batch)?; + writer.close()?; + Ok(filename) + } + + /// A one-column batch `name: int32` holding 1 and 2, with `metadata` on the field. + fn int_batch(name: &str, metadata: HashMap) -> RecordBatch { + let schema = Arc::new(Schema::new(vec![ + Field::new(name, DataType::Int32, true).with_metadata(metadata) + ])); + RecordBatch::try_new( + schema, + vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], + ) + .unwrap() + } + + /// What the planner hands `init_datasource_exec` that the field id tests vary. + struct PlannerScan { required_schema: SchemaRef, - options: SparkParquetOptions, + data_schema: SchemaRef, + projection: Vec, + use_field_id: bool, + ignore_missing_field_id: bool, + case_sensitive: bool, + } + + impl PlannerScan { + /// A scan that reads all of `required_schema`, as an unpruned query does. + fn of(required_schema: SchemaRef) -> Self { + Self { + data_schema: Arc::clone(&required_schema), + projection: (0..required_schema.fields().len()).collect(), + required_schema, + use_field_id: false, + ignore_missing_field_id: false, + case_sensitive: false, + } + } + } + + /// Scan `filename` the way the planner does, through `init_datasource_exec`, so the reader + /// factory and the schema adapter see exactly what a Spark query hands them: `data_schema` + /// is the full read schema, `required_schema` the pruned one and `projection` picks the + /// required columns out of `data_schema`. + fn scan_file_via_planner( + filename: String, + scan: PlannerScan, + ) -> Result { + let session_ctx = Arc::new(SessionContext::new()); + let exec = init_datasource_exec( + scan.required_schema, + Some(scan.data_schema), + None, + ObjectStoreUrl::local_filesystem(), + ObjectStoreBackend::Local, + vec![vec![PartitionedFile::from_path(filename)?]], + Some(scan.projection), + None, + None, + "UTC", + scan.case_sensitive, + true, + false, + false, + &session_ctx, + false, + scan.use_field_id, + scan.ignore_missing_field_id, + ) + .expect("planning the scan"); + exec.execute(0, session_ctx.task_ctx()) + } + + /// The message of the error a scan raises, either while planning or on its first poll. + async fn first_poll_error( + stream: Result, ) -> String { - match scan_parquet(batch, required_schema, options) { + match stream { Err(err) => err.to_string(), Ok(mut stream) => stream .next() @@ -2901,24 +2990,20 @@ mod test { /// rejection fires even when `fieldId.read.enabled` is off. A case-sensitive session keeps /// the name remap out of the picture. #[tokio::test] - async fn missing_file_field_ids_rejected_when_id_matching_disabled() { - let file_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); - let batch = RecordBatch::try_new( - file_schema, - vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], - ) - .unwrap(); + async fn missing_file_field_ids_rejected_when_id_matching_disabled( + ) -> Result<(), DataFusionError> { + let batch = int_batch("a", HashMap::new()); let required_schema = Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) ])); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = false; - options.case_sensitive = true; - let msg = scan_error_message(&batch, required_schema, options).await; - assert!( - msg.contains("Parquet file schema doesn't contain any field Ids"), - "unexpected error: {msg}" - ); + let scan = PlannerScan { + case_sensitive: true, + ..PlannerScan::of(required_schema) + }; + let msg = + first_poll_error(scan_file_via_planner(write_parquet(&batch, false)?, scan)).await; + assert!(msg.contains(MISSING_IDS), "unexpected error: {msg}"); + Ok(()) } /// A read schema whose only ids sit on struct children still expects ids, as Spark's @@ -2932,19 +3017,20 @@ mod test { let required_schema = struct_schema(vec![ Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")) ]); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = true; - let msg = scan_error_message(&batch, required_schema, options).await; - assert!( - msg.contains("Parquet file schema doesn't contain any field Ids"), - "unexpected error: {msg}" - ); + let scan = PlannerScan { + use_field_id: true, + ..PlannerScan::of(required_schema) + }; + let msg = + first_poll_error(scan_file_via_planner(write_parquet(&batch, false)?, scan)).await; + assert!(msg.contains(MISSING_IDS), "unexpected error: {msg}"); Ok(()) } /// Ids that only appear below the root of the file still count as ids, as in Spark's /// `containsFieldIds`. The read passes the missing-id check, resolves the nested field by - /// id, and null-fills a root field whose id the file does not hold. + /// id, and null-fills a root field whose id the file does not hold. The second round writes + /// the file without an Arrow schema hint, so the ids come from the Parquet schema alone. #[tokio::test] async fn nested_file_field_ids_satisfy_the_missing_id_check() -> Result<(), DataFusionError> { let batch = nested_id_batch()?; @@ -2958,14 +3044,23 @@ mod test { ), Field::new("missing", DataType::Int32, true).with_metadata(id_meta("7")), ])); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = true; - let mut stream = scan_parquet(&batch, required_schema, options)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.num_rows(), 2); - let s = result.column(0).as_struct(); - assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); - assert_eq!(result.column(1).null_count(), 2); + for skip_arrow_metadata in [false, true] { + let scan = PlannerScan { + use_field_id: true, + ..PlannerScan::of(Arc::clone(&required_schema)) + }; + let mut stream = + scan_file_via_planner(write_parquet(&batch, skip_arrow_metadata)?, scan)?; + let result = stream.next().await.unwrap()?; + assert_eq!( + result.num_rows(), + 2, + "skip_arrow_metadata={skip_arrow_metadata}" + ); + let s = result.column(0).as_struct(); + assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); + assert_eq!(result.column(1).null_count(), 2); + } Ok(()) } @@ -2974,18 +3069,16 @@ mod test { #[tokio::test] async fn missing_file_field_ids_allowed_when_ignore_missing_is_set( ) -> Result<(), DataFusionError> { - let file_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); - let batch = RecordBatch::try_new( - file_schema, - vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], - )?; + let batch = int_batch("a", HashMap::new()); let required_schema = Arc::new(Schema::new(vec![ Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) ])); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = true; - options.ignore_missing_field_id = true; - let mut stream = scan_parquet(&batch, required_schema, options)?; + let scan = PlannerScan { + use_field_id: true, + ignore_missing_field_id: true, + ..PlannerScan::of(required_schema) + }; + let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; let result = stream.next().await.unwrap()?; assert_eq!(result.num_rows(), 2); assert_eq!(result.column(0).null_count(), 2); @@ -2997,20 +3090,12 @@ mod test { /// the file holds a field with the same id. #[tokio::test] async fn file_ids_ignored_when_id_matching_disabled() -> Result<(), DataFusionError> { - let file_schema = Arc::new(Schema::new(vec![ - Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) - ])); - let batch = RecordBatch::try_new( - file_schema, - vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], - )?; + let batch = int_batch("a", id_meta("1")); let required_schema = Arc::new(Schema::new(vec![ Field::new("b", DataType::Int32, true).with_metadata(id_meta("1")) ])); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = false; - options.ignore_missing_field_id = false; - let mut stream = scan_parquet(&batch, required_schema, options)?; + let scan = PlannerScan::of(required_schema); + let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; let result = stream.next().await.unwrap()?; assert_eq!(result.num_rows(), 2); assert_eq!(result.column(0).null_count(), 2); @@ -3021,17 +3106,13 @@ mod test { /// The file name differs only in case so the adapter is still created. #[tokio::test] async fn no_logical_field_ids_never_rejects() -> Result<(), DataFusionError> { - let file_schema = Arc::new(Schema::new(vec![Field::new("A", DataType::Int32, true)])); - let batch = RecordBatch::try_new( - file_schema, - vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], - )?; + let batch = int_batch("A", HashMap::new()); let required_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); - let mut options = SparkParquetOptions::new(EvalMode::Legacy, "UTC", false); - options.use_field_id = true; - options.ignore_missing_field_id = false; - options.case_sensitive = false; - let mut stream = scan_parquet(&batch, required_schema, options)?; + let scan = PlannerScan { + use_field_id: true, + ..PlannerScan::of(required_schema) + }; + let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; let result = stream.next().await.unwrap()?; assert_eq!( result.column(0).as_primitive::().values(), @@ -3040,6 +3121,342 @@ mod test { Ok(()) } + /// Spark checks for missing file ids against the pruned read schema, so an id on a column + /// the query never projects does not reject a file without ids, whatever the read flag + /// says. The same read with both columns projected still raises. + #[tokio::test] + async fn field_ids_on_an_unprojected_column_do_not_reject_a_file_without_ids( + ) -> Result<(), DataFusionError> { + let file_schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, true), + Field::new("b", DataType::Int32, true), + ])); + let batch = RecordBatch::try_new( + file_schema, + vec![ + Arc::new(Int32Array::from(vec![1, 3])) as ArrayRef, + Arc::new(Int32Array::from(vec![2, 4])) as ArrayRef, + ], + )?; + let filename = write_parquet(&batch, false)?; + let a = Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")); + let b = Field::new("b", DataType::Int32, true); + let read_schema = Arc::new(Schema::new(vec![a, b.clone()])); + let pruned_schema = Arc::new(Schema::new(vec![b])); + + for use_field_id in [false, true] { + let scan = PlannerScan { + required_schema: Arc::clone(&pruned_schema), + projection: vec![1], + use_field_id, + ..PlannerScan::of(Arc::clone(&read_schema)) + }; + let mut stream = scan_file_via_planner(filename.clone(), scan)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.num_columns(), 1, "use_field_id={use_field_id}"); + assert_eq!( + result.column(0).as_primitive::().values(), + &[2, 4], + "use_field_id={use_field_id}" + ); + + let scan = PlannerScan { + use_field_id, + ..PlannerScan::of(Arc::clone(&read_schema)) + }; + let msg = first_poll_error(scan_file_via_planner(filename.clone(), scan)).await; + assert!( + msg.contains(MISSING_IDS), + "use_field_id={use_field_id}: unexpected error: {msg}" + ); + } + Ok(()) + } + + /// 2000-01-01T00:00:00Z as an INT96 value: no nanoseconds into Julian day 2451545. + fn int96_epoch_2000() -> Int96 { + let mut value = Int96::new(); + value.set_data(0, 0, 2_451_545); + value + } + + /// 2000-01-01T00:00:00Z in microseconds since the Unix epoch. + const EPOCH_2000_MICROS: i64 = 946_684_800_000_000; + + /// Write `message schema { required group s { required int32 a; required int96 ts; } }` with + /// rows (1, 2000-01-01) and (2, 2000-01-01) through the low level writer, since the Arrow + /// writer cannot produce INT96. `id` goes on the group `s` and nowhere else. + fn write_struct_with_int96(id: Option) -> Result { + let a = Arc::new( + ParquetType::primitive_type_builder("a", PhysicalType::INT32) + .with_repetition(Repetition::REQUIRED) + .build()?, + ); + let ts = Arc::new( + ParquetType::primitive_type_builder("ts", PhysicalType::INT96) + .with_repetition(Repetition::REQUIRED) + .build()?, + ); + let s = Arc::new( + ParquetType::group_type_builder("s") + .with_repetition(Repetition::REQUIRED) + .with_id(id) + .with_fields(vec![a, ts]) + .build()?, + ); + let schema = Arc::new( + ParquetType::group_type_builder("schema") + .with_fields(vec![s]) + .build()?, + ); + let filename = temp_filename(); + let file = File::create(&filename)?; + let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; + let mut row_group = writer.next_row_group()?; + let mut column = row_group.next_column()?.unwrap(); + column + .typed::() + .write_batch(&[1, 2], None, None)?; + column.close()?; + let mut column = row_group.next_column()?.unwrap(); + column.typed::().write_batch( + &[int96_epoch_2000(), int96_epoch_2000()], + None, + None, + )?; + column.close()?; + row_group.close()?; + writer.close()?; + Ok(filename) + } + + /// Read schema `s (id 1): struct`, the shape Spark requests for a + /// struct holding a timestamp column. + fn struct_with_timestamp_schema() -> SchemaRef { + Arc::new(Schema::new(vec![Field::new( + "s", + DataType::Struct(Fields::from(vec![ + Field::new("a", DataType::Int32, true), + Field::new( + "ts", + DataType::Timestamp(TimeUnit::Microsecond, Some("UTC".into())), + true, + ), + ])), + true, + ) + .with_metadata(id_meta("1"))])) + } + + /// A struct holding an INT96 timestamp reaches the schema adapter without its id, because + /// the INT96 coercion rebuilds every container field and copies no metadata. The missing-id + /// check reads the Parquet schema itself, where the id is, so the file is not rejected and + /// the struct resolves by name. Resolving it by id is a matter for the remap, which still + /// works from the coerced Arrow schema, so id matching stays off here. + #[tokio::test] + async fn struct_id_hidden_by_int96_coercion_satisfies_the_missing_id_check( + ) -> Result<(), DataFusionError> { + let filename = write_struct_with_int96(Some(1))?; + let scan = PlannerScan::of(struct_with_timestamp_schema()); + let mut stream = scan_file_via_planner(filename, scan)?; + let result = stream.next().await.unwrap()?; + let s = result.column(0).as_struct(); + assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); + assert_eq!( + s.column(1) + .as_primitive::() + .values(), + &[EPOCH_2000_MICROS, EPOCH_2000_MICROS] + ); + Ok(()) + } + + /// The same struct written without any id is still rejected under the id-bearing read + /// schema, so the check reads the Parquet schema rather than passing every INT96 file. + #[tokio::test] + async fn struct_with_int96_and_no_ids_is_rejected() -> Result<(), DataFusionError> { + let filename = write_struct_with_int96(None)?; + for use_field_id in [false, true] { + let scan = PlannerScan { + use_field_id, + ..PlannerScan::of(struct_with_timestamp_schema()) + }; + let msg = first_poll_error(scan_file_via_planner(filename.clone(), scan)).await; + assert!( + msg.contains(MISSING_IDS), + "use_field_id={use_field_id}: unexpected error: {msg}" + ); + } + Ok(()) + } + + /// Write `message schema { optional group l (LIST) { repeated group list { required int32 + /// element; } } }` with rows [1] and [2]. The only id, 5, sits on the repeated `list` + /// group, which the Arrow schema never shows. + fn write_list_with_id_on_list_group() -> Result { + let element = Arc::new( + ParquetType::primitive_type_builder("element", PhysicalType::INT32) + .with_repetition(Repetition::REQUIRED) + .build()?, + ); + let list = Arc::new( + ParquetType::group_type_builder("list") + .with_repetition(Repetition::REPEATED) + .with_id(Some(5)) + .with_fields(vec![element]) + .build()?, + ); + let l = Arc::new( + ParquetType::group_type_builder("l") + .with_repetition(Repetition::OPTIONAL) + .with_logical_type(Some(LogicalType::List)) + .with_fields(vec![list]) + .build()?, + ); + let schema = Arc::new( + ParquetType::group_type_builder("schema") + .with_fields(vec![l]) + .build()?, + ); + let filename = temp_filename(); + let file = File::create(&filename)?; + let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; + let mut row_group = writer.next_row_group()?; + let mut column = row_group.next_column()?.unwrap(); + column + .typed::() + .write_batch(&[1, 2], Some(&[2, 2]), Some(&[0, 0]))?; + column.close()?; + row_group.close()?; + writer.close()?; + Ok(filename) + } + + /// Spark's `containsFieldIds` walks the raw Parquet schema, where an id may sit on the + /// repeated `list` group that the Arrow schema folds away. Such a file is not rejected. With + /// id matching off the list resolves by name. With it on, the root field asks for id 5, no + /// root field of the file carries it, and the column is null filled, as Spark does. + #[tokio::test] + async fn id_only_on_the_list_group_satisfies_the_missing_id_check( + ) -> Result<(), DataFusionError> { + let filename = write_list_with_id_on_list_group()?; + let read_schema = Arc::new(Schema::new(vec![Field::new( + "l", + DataType::List(Arc::new(Field::new("element", DataType::Int32, true))), + true, + ) + .with_metadata(id_meta("5"))])); + + let scan = PlannerScan::of(Arc::clone(&read_schema)); + let mut stream = scan_file_via_planner(filename.clone(), scan)?; + let result = stream.next().await.unwrap()?; + let l = result.column(0).as_list::(); + assert_eq!(l.len(), 2); + assert_eq!(l.values().as_primitive::().values(), &[1, 2]); + + let scan = PlannerScan { + use_field_id: true, + ..PlannerScan::of(read_schema) + }; + let mut stream = scan_file_via_planner(filename, scan)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.column(0).null_count(), 2); + Ok(()) + } + + /// Write `message schema { optional group m (MAP) { repeated group key_value { required + /// int32 key; optional int32 value; } } }` with rows {1: 10} and {2: 20}. The only id, 5, + /// sits on the repeated `key_value` group, which the Arrow schema never shows. + fn write_map_with_id_on_key_value_group() -> Result { + let key = Arc::new( + ParquetType::primitive_type_builder("key", PhysicalType::INT32) + .with_repetition(Repetition::REQUIRED) + .build()?, + ); + let value = Arc::new( + ParquetType::primitive_type_builder("value", PhysicalType::INT32) + .with_repetition(Repetition::OPTIONAL) + .build()?, + ); + let key_value = Arc::new( + ParquetType::group_type_builder("key_value") + .with_repetition(Repetition::REPEATED) + .with_id(Some(5)) + .with_fields(vec![key, value]) + .build()?, + ); + let m = Arc::new( + ParquetType::group_type_builder("m") + .with_repetition(Repetition::OPTIONAL) + .with_logical_type(Some(LogicalType::Map)) + .with_fields(vec![key_value]) + .build()?, + ); + let schema = Arc::new( + ParquetType::group_type_builder("schema") + .with_fields(vec![m]) + .build()?, + ); + let filename = temp_filename(); + let file = File::create(&filename)?; + let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; + let mut row_group = writer.next_row_group()?; + let mut column = row_group.next_column()?.unwrap(); + column + .typed::() + .write_batch(&[1, 2], Some(&[2, 2]), Some(&[0, 0]))?; + column.close()?; + let mut column = row_group.next_column()?.unwrap(); + column + .typed::() + .write_batch(&[10, 20], Some(&[3, 3]), Some(&[0, 0]))?; + column.close()?; + row_group.close()?; + writer.close()?; + Ok(filename) + } + + /// The map counterpart of the list case: an id on the repeated `key_value` group, which the + /// Arrow schema folds away, still counts for Spark's `containsFieldIds`. With id matching + /// off the map resolves by name. With it on, the root field asks for id 5, no root field of + /// the file carries it, and the column is null filled, as Spark does. + #[tokio::test] + async fn id_only_on_the_key_value_group_satisfies_the_missing_id_check( + ) -> Result<(), DataFusionError> { + let filename = write_map_with_id_on_key_value_group()?; + let entries = Field::new( + "key_value", + DataType::Struct(Fields::from(vec![ + Field::new("key", DataType::Int32, false), + Field::new("value", DataType::Int32, true), + ])), + false, + ); + let read_schema = Arc::new(Schema::new(vec![Field::new( + "m", + DataType::Map(Arc::new(entries), false), + true, + ) + .with_metadata(id_meta("5"))])); + + let scan = PlannerScan::of(Arc::clone(&read_schema)); + let mut stream = scan_file_via_planner(filename.clone(), scan)?; + let result = stream.next().await.unwrap()?; + let m = result.column(0).as_map(); + assert_eq!(m.len(), 2); + assert_eq!(m.keys().as_primitive::().values(), &[1, 2]); + assert_eq!(m.values().as_primitive::().values(), &[10, 20]); + + let scan = PlannerScan { + use_field_id: true, + ..PlannerScan::of(read_schema) + }; + let mut stream = scan_file_via_planner(filename, scan)?; + let result = stream.next().await.unwrap()?; + assert_eq!(result.column(0).null_count(), 2); + Ok(()) + } + /// Disallowed widening (`INT32 -> bigint` with `allow_type_promotion` off) inside a /// struct defers to `RejectOnNonEmpty`, like the top level: a non-empty file fails ... #[tokio::test] diff --git a/native/jni-bridge/src/errors.rs b/native/jni-bridge/src/errors.rs index 3e72f6e8048..6cc9299c3f1 100644 --- a/native/jni-bridge/src/errors.rs +++ b/native/jni-bridge/src/errors.rs @@ -573,7 +573,9 @@ fn throw_exception(env: &mut Env, error: &CometError, backtrace: Option) // FAILED_READ_FILE / FileNotFound via the structured SparkError channel. Anything else // falls back to generic handling. CometError::DataFusion { msg: _, source } => { - if let Some(spark_error) = try_classify_file_read_error(source) { + if let Some(spark_error) = parquet_external_spark_error(source) { + throw_spark_error_as_json(env, spark_error) + } else if let Some(spark_error) = try_classify_file_read_error(source) { throw_spark_error_as_json(env, &spark_error) } else { throw_generic_exception(env, error, backtrace) @@ -646,6 +648,24 @@ fn throw_spark_error_as_json(env: &mut Env, spark_error: &SparkError) -> jni::er ) } +/// A `SparkError` raised by the Parquet reader while opening a file, such as the missing field +/// id check in the reader factory's `get_metadata`, arrives as +/// `DataFusionError::ParquetError(ParquetError::External(spark_error))`. Unwrap it so the error +/// keeps its own JVM exception class instead of being classified as a file read failure. +/// `Context` and `Shared` wrappers are looked through, as `try_classify_file_read_error` does. +fn parquet_external_spark_error(error: &DataFusionError) -> Option<&SparkError> { + use datafusion::common::DataFusionError as DFE; + match error { + DFE::ParquetError(pe) => match pe.as_ref() { + ParquetError::External(inner) => inner.downcast_ref::(), + _ => None, + }, + DFE::Context(_, inner) => parquet_external_spark_error(inner), + DFE::Shared(inner) => parquet_external_spark_error(inner), + _ => None, + } +} + /// Classify a `DataFusionError` as a per-file read failure by TYPED variant (not message text), /// returning `SparkError::CannotReadFile` if so. This is the structured replacement for the /// previous JVM-side substring matching on error prose. @@ -1370,6 +1390,34 @@ mod tests { } } + /// A `SparkError` the Parquet reader raised on open stays typed through the `ParquetError` + /// wrapper and through `Context` and `Shared` wrappers, while an ordinary reader error does + /// not match. + #[test] + fn parquet_external_spark_error_keeps_its_type() { + let raised = DataFusionError::ParquetError(Box::new(ParquetError::External(Box::new( + SparkError::ParquetMissingFieldIds, + )))); + assert!(matches!( + parquet_external_spark_error(&raised), + Some(SparkError::ParquetMissingFieldIds) + )); + let wrapped = DataFusionError::Context("open".to_string(), Box::new(raised)); + assert!(matches!( + parquet_external_spark_error(&wrapped), + Some(SparkError::ParquetMissingFieldIds) + )); + let shared = DataFusionError::Shared(Arc::new(wrapped)); + assert!(matches!( + parquet_external_spark_error(&shared), + Some(SparkError::ParquetMissingFieldIds) + )); + let corrupt = DataFusionError::ParquetError(Box::new(ParquetError::General( + "corrupt footer".to_string(), + ))); + assert!(parquet_external_spark_error(&corrupt).is_none()); + } + #[test] fn classify_parquet_error_is_file_read() { let e = DataFusionError::ParquetError(Box::new(parquet::errors::ParquetError::General( 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 b4e4ce68b1e..ca19da85236 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -21,6 +21,7 @@ package org.apache.comet.parquet import java.io.File import java.math.{BigDecimal, BigInteger} +import java.sql.Timestamp import java.time.{ZoneId, ZoneOffset} import java.util.{Base64, Collections} @@ -2171,20 +2172,13 @@ abstract class ParquetReadSuite extends CometTestBase { def readCause(): Throwable = intercept[SparkException] { spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() }.getCause - def assertMissingIds(cause: Throwable): Unit = { - assert( - cause.isInstanceOf[RuntimeException] && - cause.getMessage.contains("Parquet file schema doesn't contain any field Ids"), - cause) - } - withClue("Spark with Comet disabled") { withSQLConf(CometConf.COMET_ENABLED.key -> "false") { - assertMissingIds(readCause()) + assertMissingIdsException(readCause()) } } withClue("Comet") { - assertMissingIds(readCause()) + assertMissingIdsException(readCause()) } withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { @@ -2245,6 +2239,209 @@ abstract class ParquetReadSuite extends CometTestBase { } } } + + // Spark checks for missing file ids against the pruned read schema, so an id on a column + // the query never projects does not reject a file without ids, whatever the read flag says. + // The same read with both columns projected still raises. + test("field ids on an unprojected column do not reject a file without ids") { + Seq("false", "true").foreach { readEnabled => + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { + withTempPath { dir => + val writeSchema = new StructType().add("a", IntegerType).add("b", IntegerType) + val readSchema = new StructType() + .add("a", IntegerType, true, withId(1)) + .add("b", IntegerType, true) + val writeData = Seq(Row(1, 2), Row(3, 4)) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + withClue(s"read flag $readEnabled") { + val pruned = spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("b") + checkSparkAnswerAndOperator(pruned) + checkAnswer(pruned, Row(2) :: Row(4) :: Nil) + + val cause = intercept[SparkException] { + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() + }.getCause + assert( + cause.isInstanceOf[RuntimeException] && + cause.getMessage.contains("Parquet file schema doesn't contain any field Ids"), + cause) + } + } + } + } + } + + // Spark writes timestamps as INT96 by default. The reader coerces INT96 to microseconds and + // rebuilds every container field without its metadata on the way, so the id on `s` is gone + // from the Arrow schema the adapter sees. The missing-id check reads the Parquet schema, where + // the id still is, so the file reads and `s` resolves by name. Resolving `s` by id is a + // matter for the remap, which still works from the coerced schema, so id matching stays off. + test("field ids on a struct holding a timestamp survive the missing-id check") { + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { + withTempPath { dir => + val nested = new StructType().add("a", IntegerType).add("ts", TimestampType) + val schema = new StructType().add("s", nested, true, withId(1)) + val ts = Timestamp.valueOf("2020-01-01 00:00:00") + val writeData = Seq(Row(Row(1, ts)), Row(Row(2, ts))) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), schema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + val df = spark.read.schema(schema).parquet(dir.getCanonicalPath) + checkSparkAnswerAndOperator(df) + checkAnswer(df, Row(Row(1, ts)) :: Row(Row(2, ts)) :: Nil) + } + } + } + + // Spark checks each file on its own. A directory holding one file with ids and one without + // raises on the second, and with `ignoreMissing` the file without ids reads as nulls because + // no root field of it carries the requested id. + test("a file without ids next to a file with ids is checked on its own") { + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "true") { + withTempPath { dir => + val idSchema = new StructType().add("x", IntegerType, true, withId(1)) + val plainSchema = new StructType().add("a", IntegerType, true) + val readSchema = new StructType().add("a", IntegerType, true, withId(1)) + spark + .createDataFrame(spark.sparkContext.parallelize(Seq(Row(100), Row(200))), idSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + spark + .createDataFrame(spark.sparkContext.parallelize(Seq(Row(1), Row(2))), plainSchema) + .write + .mode("append") + .parquet(dir.getCanonicalPath) + + val cause = intercept[SparkException] { + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() + }.getCause + assertMissingIdsException(cause) + + withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { + val df = spark.read.schema(readSchema).parquet(dir.getCanonicalPath) + checkSparkAnswerAndOperator(df) + checkAnswer(df, Row(100) :: Row(200) :: Row(null) :: Row(null) :: Nil) + } + } + } + } + + // Nested schema pruning hands the reader only the struct children a query touches, so an id + // on `s.a` rejects a file without ids when `s.a` is read and plays no part when only `s.b` is. + test("field ids on a pruned struct child are checked only when that child is read") { + Seq("false", "true").foreach { readEnabled => + withSQLConf( + SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled, + SQLConf.NESTED_SCHEMA_PRUNING_ENABLED.key -> "true") { + withTempPath { dir => + val writeSchema = new StructType() + .add("s", new StructType().add("a", IntegerType).add("b", IntegerType), true) + val readSchema = new StructType().add( + "s", + StructType( + Seq( + StructField("a", IntegerType, nullable = true, withId(11)), + StructField("b", IntegerType, nullable = true))), + true) + spark + .createDataFrame( + spark.sparkContext.parallelize(Seq(Row(Row(1, 2)), Row(Row(3, 4)))), + writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + withClue(s"read flag $readEnabled") { + val cause = intercept[SparkException] { + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("s.a").collect() + }.getCause + assertMissingIdsException(cause) + + val onlyB = spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("s.b") + checkSparkAnswerAndOperator(onlyB) + checkAnswer(onlyB, Row(2) :: Row(4) :: Nil) + } + } + } + } + } + + // Id zero is an id like any other, so a read schema carrying it expects ids in the file. + test("field id zero on a root field expects ids in the file") { + Seq("false", "true").foreach { readEnabled => + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { + withTempPath { dir => + val readSchema = new StructType().add("a", IntegerType, true, withId(0)) + val writeSchema = new StructType().add("a", IntegerType, true) + spark + .createDataFrame(spark.sparkContext.parallelize(Seq(Row(1), Row(2))), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + withClue(s"read flag $readEnabled") { + def readCause(): Throwable = intercept[SparkException] { + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() + }.getCause + withSQLConf(CometConf.COMET_ENABLED.key -> "false") { + assertMissingIdsException(readCause()) + } + assertMissingIdsException(readCause()) + } + } + } + } + } + + // A count reads no columns, so the pruned read schema carries no ids and a file without ids + // is counted rather than rejected. + test("count over an id-bearing schema reads a file without ids") { + Seq("false", "true").foreach { readEnabled => + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { + withTempPath { dir => + val readSchema = new StructType().add("a", IntegerType, true, withId(1)) + val writeSchema = new StructType().add("a", IntegerType, true) + spark + .createDataFrame(spark.sparkContext.parallelize(Seq(Row(1), Row(2))), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) + + withClue(s"read flag $readEnabled") { + val counted = + spark.read.schema(readSchema).parquet(dir.getCanonicalPath).selectExpr("count(*)") + checkSparkAnswerAndOperator(counted) + checkAnswer(counted, Row(2L) :: Nil) + } + } + } + } + } + + // Spark raises a plain `RuntimeException` for a read schema with ids over a file without any. + // Spark 4 wraps a reader failure in a `FAILED_READ_FILE` `SparkException`, and the Comet + // shim does the same, so the exception sits one level deeper there. + private def assertMissingIdsException(cause: Throwable): Unit = { + val missingIds = if (isSpark40Plus) { + assert(cause.isInstanceOf[SparkException], cause) + cause.getCause + } else { + cause + } + assert(missingIds.getClass == classOf[RuntimeException], missingIds) + assert( + missingIds.getMessage.contains("Parquet file schema doesn't contain any field Ids"), + missingIds) + } } class ParquetReadV1Suite extends ParquetReadSuite with AdaptiveSparkPlanHelper { From 0f89b1ab617c474d79c7beb89c383bef44b94b62 Mon Sep 17 00:00:00 2001 From: Dustin Smith Date: Fri, 25 Sep 2026 00:45:13 +0700 Subject: [PATCH 3/4] test: find the missing field ids error anywhere in the cause chain On Spark 4.0 and 4.1 the RuntimeException Spark raises for a read schema with field ids over a file without any arrives wrapped once, in the FAILED_READ_FILE SparkException that collect raises, and the helper assumed one more layer above it. It now walks the cause chain and requires a plain RuntimeException carrying Spark's message, for Spark and for Comet alike, so the layer count no longer matters. --- .../comet/parquet/ParquetReadSuite.scala | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 ca19da85236..ec9aa00b03c 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -2428,19 +2428,19 @@ abstract class ParquetReadSuite extends CometTestBase { } // Spark raises a plain `RuntimeException` for a read schema with ids over a file without any. - // Spark 4 wraps a reader failure in a `FAILED_READ_FILE` `SparkException`, and the Comet - // shim does the same, so the exception sits one level deeper there. + // How many `SparkException` layers sit above it varies with the Spark version. Spark 4 wraps + // a reader failure in a `FAILED_READ_FILE` `SparkException`, and on 4.0 and 4.1 that wrapper + // is the exception `collect()` raises, so the `RuntimeException` is its direct cause. The + // Comet shim follows Spark, so walk the cause chain instead of counting the layers above it. private def assertMissingIdsException(cause: Throwable): Unit = { - val missingIds = if (isSpark40Plus) { - assert(cause.isInstanceOf[SparkException], cause) - cause.getCause - } else { - cause + val chain = Iterator.iterate(cause)(_.getCause).takeWhile(_ != null).toSeq + val missingIds = chain.find { t => + t.getClass == classOf[RuntimeException] && + Option(t.getMessage).exists(_.contains("Parquet file schema doesn't contain any field Ids")) } - assert(missingIds.getClass == classOf[RuntimeException], missingIds) assert( - missingIds.getMessage.contains("Parquet file schema doesn't contain any field Ids"), - missingIds) + missingIds.isDefined, + chain.map(t => s"${t.getClass.getName}: ${t.getMessage}").mkString("\n")) } } From 59873b92cf7a8519c9ca2b6c39d5cb29f0727b64 Mon Sep 17 00:00:00 2001 From: Dustin Smith Date: Fri, 25 Sep 2026 08:32:21 +0700 Subject: [PATCH 4/4] fix: let the serde say whether the file must carry field ids The requested side of the missing field ids check is Spark's own hasFieldIds over the required schema, so the serde now sends one flag, require_field_ids, in place of ignore_missing_field_id, and the native Arrow walk is gone. The reader factory takes that one flag through a builder and the parquet options carry nothing for it. The error names the file, and the module doc leads with the lasting reason the check reads the raw footer: ids on repeated groups and on the message root never reach Arrow. The Rust scan tests that had Scala twins are gone, the repeated group cases moved to Scala where they compare against Spark, and the Scala tests fold into the existing port and one pruned schema test with Spark's own assertion form. --- native/common/src/error.rs | 30 +- native/core/src/execution/planner.rs | 2 +- .../eager_page_index_reader_factory.rs | 182 ++++-- native/core/src/parquet/parquet_exec.rs | 15 +- native/core/src/parquet/parquet_support.rs | 114 +--- native/core/src/parquet/schema_adapter.rs | 618 +----------------- native/jni-bridge/src/errors.rs | 13 +- native/proto/src/proto/operator.proto | 7 +- .../serde/operator/CometNativeScan.scala | 11 +- .../comet/parquet/ParquetReadSuite.scala | 359 ++++------ 10 files changed, 300 insertions(+), 1051 deletions(-) diff --git a/native/common/src/error.rs b/native/common/src/error.rs index 41773237cba..89a5e1a7c59 100644 --- a/native/common/src/error.rs +++ b/native/common/src/error.rs @@ -228,11 +228,12 @@ pub enum SparkError { matched_fields: String, }, - /// The read schema requests Parquet field-id matching but the file carries no field ids. - /// Mirrors the runtime error raised in Spark's `ParquetReadSupport` when - /// `spark.sql.parquet.fieldId.read.ignoreMissing` is false. + /// The read schema carries Parquet field ids but the file carries none. Mirrors the runtime + /// error raised in Spark's `ParquetReadSupport` when + /// `spark.sql.parquet.fieldId.read.ignoreMissing` is false. Spark's message names no file, + /// so `file_path` travels as a parameter for the 4.x shim's `FAILED_READ_FILE` wrapper. #[error("Spark read schema expects field Ids, but Parquet file schema doesn't contain any field Ids. Please remove the field ids from Spark schema or ignore missing ids by setting `spark.sql.parquet.fieldId.read.ignoreMissing = true`")] - ParquetMissingFieldIds, + ParquetMissingFieldIds { file_path: String }, /// Schema mismatch when reading a Parquet column under a requested schema /// that's incompatible with the physical column type. Translated by the JVM @@ -347,7 +348,7 @@ impl SparkError { SparkError::FileNotFound { .. } => "FileNotFound", SparkError::DuplicateFieldCaseInsensitive { .. } => "DuplicateFieldCaseInsensitive", SparkError::DuplicateFieldByFieldId { .. } => "DuplicateFieldByFieldId", - SparkError::ParquetMissingFieldIds => "ParquetMissingFieldIds", + SparkError::ParquetMissingFieldIds { .. } => "ParquetMissingFieldIds", SparkError::ParquetSchemaConvert { .. } => "ParquetSchemaConvert", SparkError::CannotReadFile { .. } => "CannotReadFile", SparkError::Arrow(_) => "Arrow", @@ -598,6 +599,11 @@ impl SparkError { "matchedFields": matched_fields, }) } + SparkError::ParquetMissingFieldIds { file_path } => { + serde_json::json!({ + "filePath": file_path, + }) + } SparkError::ParquetSchemaConvert { file_path, column, @@ -704,10 +710,11 @@ impl SparkError { // (Spark's `foundDuplicateFieldInFieldIdLookupModeError` returns SparkRuntimeException) SparkError::DuplicateFieldByFieldId { .. } => "org/apache/spark/SparkRuntimeException", - // ParquetMissingFieldIds - converted to a plain RuntimeException by the shim, - // matching the `RuntimeException` Spark's ParquetReadSupport throws when the - // file lacks field ids and `spark.sql.parquet.fieldId.read.ignoreMissing=false`. - SparkError::ParquetMissingFieldIds => "java/lang/RuntimeException", + // ParquetMissingFieldIds - converted to the plain RuntimeException Spark's + // ParquetReadSupport throws when the file lacks field ids and + // `spark.sql.parquet.fieldId.read.ignoreMissing=false`. The 4.x shim wraps it in + // the FAILED_READ_FILE SparkException Spark 4 raises at the task boundary. + SparkError::ParquetMissingFieldIds { .. } => "java/lang/RuntimeException", // ParquetSchemaConvert - converted to SchemaColumnConvertNotSupportedException by the shim SparkError::ParquetSchemaConvert { .. } => { @@ -806,8 +813,9 @@ impl SparkError { // Duplicate field id in id-lookup mode SparkError::DuplicateFieldByFieldId { .. } => Some("_LEGACY_ERROR_TEMP_2094"), - // ParquetMissingFieldIds is a plain RuntimeException with no error class. - SparkError::ParquetMissingFieldIds => None, + // ParquetMissingFieldIds is a plain RuntimeException with no error class. The 4.x + // shim supplies FAILED_READ_FILE itself, so none is exposed here. + SparkError::ParquetMissingFieldIds { .. } => None, // Parquet schema mismatch — translated to SchemaColumnConvertNotSupportedException // by the JVM shim. The shim wraps it in the version-appropriate diff --git a/native/core/src/execution/planner.rs b/native/core/src/execution/planner.rs index 7123bb1b1f2..a93015e653f 100644 --- a/native/core/src/execution/planner.rs +++ b/native/core/src/execution/planner.rs @@ -1739,7 +1739,7 @@ impl PhysicalPlanner { self.session_ctx(), common.encryption_enabled, common.use_field_id, - common.ignore_missing_field_id, + common.require_field_ids, )?; Ok(( vec![], 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 0538ef48383..cadcf4c9f4e 100644 --- a/native/core/src/parquet/eager_page_index_reader_factory.rs +++ b/native/core/src/parquet/eager_page_index_reader_factory.rs @@ -43,15 +43,22 @@ //! fetch to encrypted scans that have no pruning predicate at all. Encrypted opens get exactly //! the caller's requested policy, unchanged from stock behavior. //! -//! Filed upstream as apache/datafusion#23978. Revert this once the opener merges its deferred -//! page-index load back into `FileMetadataCache` instead of bypassing it. +//! Filed upstream as apache/datafusion#23978. Once the opener merges its deferred page-index +//! load back into `FileMetadataCache` instead of bypassing it, the eager policy can go, but the +//! factory cannot: `get_metadata` is the one per-file hook that sees the raw footer, and two +//! other things hang off it. //! -//! The reader also carries Spark's missing field id check, because the footer is first at hand -//! here. Spark's `ParquetReadSupport` refuses to open a file whose raw schema carries no field -//! id when the requested schema carries one, unless `ignoreMissing` is set, and it walks the -//! raw `MessageType` to decide. The Arrow schema the schema adapter sees later cannot stand in -//! for that walk: the INT96 coercion rebuilds container fields without their metadata, and an -//! id on a `list` or `key_value` group, or on the message root, never reaches an Arrow field. +//! The first is Spark's missing field id check. `ParquetReadSupport` refuses to open a file +//! whose Parquet schema carries no field id when the requested schema carries one, unless +//! `ignoreMissing` is set, and it walks the raw `MessageType` to decide. That walk has to run +//! over the Parquet schema rather than the Arrow schema the schema adapter is handed later, +//! because an id on a repeated `list` or `key_value` group, or on the message root, never +//! reaches an Arrow field. Until apache/datafusion#24790, which is not in DataFusion 55.1.0, +//! the INT96 coercion also rebuilt container fields without their metadata, so a struct id could +//! vanish on the way to Arrow as well. +//! +//! The second is the Variant footer rewrite, `with_spark_arrow_schema`, which replaces the +//! Arrow schema hint in the footer for scans that project Variant. use arrow::datatypes::{DataType, FieldRef, Schema}; use async_trait::async_trait; @@ -171,11 +178,9 @@ 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, - // Whether the schema Spark asked the scan for carries a field id at any depth, computed - // once by the planner. Together with `ignore_missing_field_id` it decides whether a file - // whose Parquet schema carries no id is refused, as Spark's `ParquetReadSupport` does. - requested_schema_has_field_ids: bool, - ignore_missing_field_id: 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, } impl EagerPageIndexReaderFactory { @@ -204,8 +209,7 @@ impl EagerPageIndexReaderFactory { metadata_cache, scan_io_metrics, spark_variant_schema: false, - requested_schema_has_field_ids: false, - ignore_missing_field_id: false, + require_field_ids: false, } } @@ -214,30 +218,23 @@ impl EagerPageIndexReaderFactory { self } - /// Arm Spark's missing field id check. A file whose Parquet schema carries no field id - /// is refused on open when `requested_schema_has_field_ids` is set and - /// `ignore_missing_field_id` is not. Both default to off, so a factory that never calls - /// this reads every file. - pub fn with_missing_field_id_check( - mut self, - requested_schema_has_field_ids: bool, - ignore_missing_field_id: bool, - ) -> Self { - self.requested_schema_has_field_ids = requested_schema_has_field_ids; - self.ignore_missing_field_id = ignore_missing_field_id; + /// Refuse a file whose Parquet schema carries no field id. Off by default, so a factory + /// that never calls this reads every file. + pub fn with_require_field_ids(mut self, enabled: bool) -> Self { + self.require_field_ids = enabled; self } } /// True when `node` or any node under it carries a field id, the way Spark's /// `containsFieldIds` answers it over the raw Parquet schema, message root included. -fn parquet_schema_has_field_ids(node: &ParquetType) -> bool { +fn contains_field_ids(node: &ParquetType) -> bool { node.get_basic_info().has_id() || (node.is_group() && node .get_fields() .iter() - .any(|field| parquet_schema_has_field_ids(field))) + .any(|field| contains_field_ids(field))) } impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { @@ -265,8 +262,7 @@ impl ParquetFileReaderFactory for EagerPageIndexReaderFactory { metadata_cache: Arc::clone(&self.metadata_cache), metadata_size_hint, spark_variant_schema: self.spark_variant_schema, - requested_schema_has_field_ids: self.requested_schema_has_field_ids, - ignore_missing_field_id: self.ignore_missing_field_id, + require_field_ids: self.require_field_ids, })) } } @@ -282,8 +278,7 @@ struct EagerPageIndexReader { metadata_cache: Arc, metadata_size_hint: Option, spark_variant_schema: bool, - requested_schema_has_field_ids: bool, - ignore_missing_field_id: bool, + require_field_ids: bool, } // Arrow infers ENUM as Binary, losing the distinction from raw binary that Spark needs. @@ -483,8 +478,7 @@ impl AsyncFileReader for EagerPageIndexReader { let metadata_size_hint = self.metadata_size_hint; let scan_io_metrics = Arc::clone(&self.scan_io_metrics); let spark_variant_schema = self.spark_variant_schema; - let require_file_field_ids = - self.requested_schema_has_field_ids && !self.ignore_missing_field_id; + let require_field_ids = self.require_field_ids; async move { let file_decryption_properties = options .and_then(|o| o.file_decryption_properties()) @@ -544,18 +538,16 @@ impl AsyncFileReader for EagerPageIndexReader { } let metadata = metadata?; - // Spark's `ParquetReadSupport` refuses to open a file that carries no field ids when - // the requested schema carries some, unless `ignoreMissing` is set, and it walks the - // raw `MessageType` to decide. The same walk runs here over the footer's schema. The - // error keeps its Spark type through `ParquetError::External`, which the JNI layer - // unwraps, so the JVM sees the same exception Spark raises. - if require_file_field_ids - && !parquet_schema_has_field_ids( - metadata.file_metadata().schema_descr().root_schema(), - ) + // Spark's missing field id check, over the raw Parquet schema as in + // `ParquetReadSupport`. The JNI layer unwraps the `External` error, so the JVM sees + // the exception Spark raises. + if require_field_ids + && !contains_field_ids(metadata.file_metadata().schema_descr().root_schema()) { return Err(ParquetError::External(Box::new( - SparkError::ParquetMissingFieldIds, + SparkError::ParquetMissingFieldIds { + file_path: object_meta.location.to_string(), + }, ))); } if spark_variant_schema { @@ -893,7 +885,7 @@ mod tests { use arrow::{array::Int32Array, record_batch::RecordBatch}; use object_store::memory::InMemory; use parquet::{ - arrow::ArrowWriter, + arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY}, file::{ properties::{EnabledStatistics, WriterProperties}, reader::FileReader, @@ -1053,9 +1045,10 @@ mod tests { } /// The raw schema walk answers like Spark's `containsFieldIds`: an id on the message root, - /// on a leaf, or on a repeated `list` group counts, and a schema without any does not. + /// on a leaf, or on a repeated `list` or `key_value` group counts, and a schema without + /// any does not. #[test] - fn parquet_schema_has_field_ids_sees_ids_on_any_node() { + fn contains_field_ids_sees_ids_on_any_node() { use parquet::basic::Type as PhysicalType; use parquet::schema::types::TypePtr; @@ -1077,22 +1070,18 @@ mod tests { ) }; - assert!(!parquet_schema_has_field_ids(&group( - "schema", - None, - vec![] - ))); - assert!(!parquet_schema_has_field_ids(&group( + assert!(!contains_field_ids(&group("schema", None, vec![]))); + assert!(!contains_field_ids(&group( "schema", None, vec![leaf("a", None)] ))); - assert!(parquet_schema_has_field_ids(&group( + assert!(contains_field_ids(&group( "schema", Some(1), vec![leaf("a", None)] ))); - assert!(parquet_schema_has_field_ids(&group( + assert!(contains_field_ids(&group( "schema", None, vec![leaf("a", Some(1))] @@ -1106,13 +1095,92 @@ mod tests { vec![group("list", Some(5), vec![leaf("element", None)])], )], ); - assert!(parquet_schema_has_field_ids(&list_group_only)); + assert!(contains_field_ids(&list_group_only)); + let key_value_group_only = group( + "schema", + None, + vec![group( + "m", + None, + vec![group( + "key_value", + Some(6), + vec![leaf("key", None), leaf("value", None)], + )], + )], + ); + assert!(contains_field_ids(&key_value_group_only)); let nested_without_ids = group( "schema", None, vec![group("s", None, vec![leaf("a", None)])], ); - assert!(!parquet_schema_has_field_ids(&nested_without_ids)); + assert!(!contains_field_ids(&nested_without_ids)); + } + + /// Write a one-column `a: int32` file into `store`, with a field id on `a` when `id` is + /// set, and return the file as `PartitionedFile`. + async fn put_int_file(store: &InMemory, location: &str, id: Option<&str>) -> PartitionedFile { + let mut field = arrow::datatypes::Field::new("a", DataType::Int32, false); + if let Some(id) = id { + field = field + .with_metadata([(PARQUET_FIELD_ID_META_KEY.to_string(), id.to_string())].into()); + } + let schema = Arc::new(Schema::new(vec![field])); + let batch = RecordBatch::try_new( + Arc::clone(&schema), + vec![Arc::new(Int32Array::from(vec![1, 2]))], + ) + .unwrap(); + let mut bytes = Vec::new(); + let mut writer = ArrowWriter::try_new(&mut bytes, schema, None).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + let size = bytes.len() as u64; + store + .put(&Path::from(location), Bytes::from(bytes).into()) + .await + .unwrap(); + PartitionedFile::new(location.to_string(), size) + } + + /// A reader made by a factory with `require_field_ids` set refuses a file without ids on + /// its first metadata fetch, naming the file, and reads one that carries an id. A factory + /// without it reads the file without ids. + #[tokio::test] + async fn get_metadata_refuses_a_file_without_ids_only_when_required() { + let store = Arc::new(InMemory::new()); + let without_ids = put_int_file(&store, "no_ids.parquet", None).await; + let with_ids = put_int_file(&store, "ids.parquet", Some("1")).await; + let runtime = datafusion::execution::runtime_env::RuntimeEnv::default(); + let metrics = ExecutionPlanMetricsSet::new(); + let metadata_for = |require: bool, file: PartitionedFile| { + let factory = EagerPageIndexReaderFactory::new( + Arc::clone(&store) as Arc, + runtime.cache_manager.get_file_metadata_cache(), + ScanIoSource::Local, + &metrics, + ) + .with_require_field_ids(require); + let mut reader = factory.create_reader(0, file, None, &metrics).unwrap(); + async move { reader.get_metadata(None).await } + }; + + let err = metadata_for(true, without_ids.clone()) + .await + .expect_err("a file without ids must be refused"); + match err { + ParquetError::External(inner) => assert!( + matches!( + inner.downcast_ref::(), + Some(SparkError::ParquetMissingFieldIds { file_path }) if file_path == "no_ids.parquet" + ), + "unexpected error: {inner}" + ), + other => panic!("unexpected error: {other}"), + } + assert!(metadata_for(true, with_ids).await.is_ok()); + assert!(metadata_for(false, without_ids).await.is_ok()); } #[test] diff --git a/native/core/src/parquet/parquet_exec.rs b/native/core/src/parquet/parquet_exec.rs index 5d2ccef46f7..df621ccbc4a 100644 --- a/native/core/src/parquet/parquet_exec.rs +++ b/native/core/src/parquet/parquet_exec.rs @@ -20,7 +20,7 @@ use crate::parquet::eager_page_index_reader_factory::{EagerPageIndexReaderFactor use crate::parquet::encryption_support::{CometEncryptionConfig, ENCRYPTION_FACTORY_ID}; use crate::parquet::name_fold::fold_schema_names; use crate::parquet::parquet_support::{ - any_nested_field_has_id, object_store_authority, ObjectStoreBackend, SparkParquetOptions, + object_store_authority, ObjectStoreBackend, SparkParquetOptions, }; use crate::parquet::schema_adapter::SparkPhysicalExprAdapterFactory; use arrow::datatypes::{Field, FieldRef, SchemaRef}; @@ -83,7 +83,7 @@ pub(crate) fn init_datasource_exec( session_ctx: &Arc, encryption_enabled: bool, use_field_id: bool, - ignore_missing_field_id: bool, + require_field_ids: bool, ) -> Result, ExecutionError> { // Computed once and reused below for `try_pushdown_filters`. `copied_config()` clones only // `SessionConfig` (an `Arc` plus a small extensions map); `SessionContext:: @@ -101,12 +101,6 @@ pub(crate) fn init_datasource_exec( &session_config.options().execution.parquet, ); spark_parquet_options.use_field_id = use_field_id; - spark_parquet_options.ignore_missing_field_id = ignore_missing_field_id; - // Spark runs its missing-id check against the pruned read schema it hands the reader, not - // the full data schema that DataFusion later passes the schema adapter, so the answer is - // taken from `required_schema` here, once per scan, and handed to the reader factory below. - spark_parquet_options.requested_schema_has_field_ids = - any_nested_field_has_id(required_schema.fields()); // Spark can discard filtered-out values before timestamp conversion using statistics, // dictionary, and row-level filters. Comet cannot mirror every pruning path, so applying // checked conversion in a filtered scan can fail on values Spark never reads. Preserve the @@ -200,10 +194,7 @@ pub(crate) fn init_datasource_exec( parquet_source.metrics(), ) .with_spark_variant_schema(projects_variant) - .with_missing_field_id_check( - spark_parquet_options.requested_schema_has_field_ids, - spark_parquet_options.ignore_missing_field_id, - ), + .with_require_field_ids(require_field_ids), ); parquet_source = parquet_source.with_parquet_file_reader_factory(reader_factory); diff --git a/native/core/src/parquet/parquet_support.rs b/native/core/src/parquet/parquet_support.rs index 27923cb4e4f..adb057c16f7 100644 --- a/native/core/src/parquet/parquet_support.rs +++ b/native/core/src/parquet/parquet_support.rs @@ -22,7 +22,7 @@ use arrow::array::{ }; use arrow::buffer::NullBuffer; use arrow::compute::can_cast_types; -use arrow::datatypes::{Field, FieldRef, Fields}; +use arrow::datatypes::{FieldRef, Fields}; use arrow::{ array::{ cast::AsArray, new_null_array, types::TimestampMicrosecondType, @@ -104,15 +104,6 @@ pub struct SparkParquetOptions { /// (mirrors Spark's `spark.sql.parquet.fieldId.read.enabled`). Only takes effect /// when both physical and logical fields actually carry IDs. pub use_field_id: bool, - /// When false (Spark's default), reading a file that has no field ids while the - /// requested schema does carry ids raises a runtime error rather than silently - /// producing nulls (mirrors `spark.sql.parquet.fieldId.read.ignoreMissing`). - pub ignore_missing_field_id: bool, - /// Whether the schema Spark asked the scan for carries a Parquet field id at any depth. - /// Spark's `ParquetReadSupport` runs its missing-id check against that pruned schema, so - /// the planner computes this once from `required_schema`. The reader factory tests it when - /// it reads a file's footer, against the Parquet schema found there. - pub requested_schema_has_field_ids: bool, /// Whether type promotion (schema evolution) is allowed, e.g. INT32 -> INT64, /// FLOAT -> DOUBLE. Mirrors spark.comet.schemaEvolution.enabled. pub allow_type_promotion: bool, @@ -140,8 +131,6 @@ impl SparkParquetOptions { case_sensitive: false, return_null_struct_if_all_fields_missing: true, use_field_id: false, - ignore_missing_field_id: false, - requested_schema_has_field_ids: false, allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, @@ -158,8 +147,6 @@ impl SparkParquetOptions { case_sensitive: false, return_null_struct_if_all_fields_missing: true, use_field_id: false, - ignore_missing_field_id: false, - requested_schema_has_field_ids: false, allow_type_promotion: false, allow_timestamp_ltz_to_ntz: false, checked_timestamp_overflow: true, @@ -425,51 +412,6 @@ fn field_id(field: &arrow::datatypes::Field) -> Option { .and_then(|v| v.parse::().ok()) } -/// True when a field in `fields`, at any nesting depth, carries a Parquet field id. Spark's -/// `containsFieldIds` walks the whole file schema the same way, and `ParquetUtils.hasFieldIds` -/// walks the read schema. The planner runs this over the requested schema once, at plan time. -/// The file side is not an Arrow walk at all: the reader factory checks the Parquet schema in -/// the footer, because the Arrow schema the adapter sees can lose ids (the INT96 coercion -/// rebuilds container fields without their metadata) and never shows an id that sits on a -/// `list` or `key_value` group. The root-only `schema_has_field_ids` in the schema adapter -/// stays as the gate for id matching, which only ever renames root fields. -pub(crate) fn any_nested_field_has_id(fields: &Fields) -> bool { - fields.iter().any(|f| field_holds_id(f)) -} - -/// Whether `field` or anything nested under it carries a Parquet field id. -/// -/// This walks the requested schema, where only struct fields can hold the metadata: Spark's -/// `hasFieldIds` recurses through `ArrayType` and `MapType` into their element and key or value -/// types, only a `StructField` carries metadata, and the serde never populates the element or -/// key and value fields. The walk still descends through list and map fields to reach the -/// structs nested inside them. The file side is checked by the reader factory over the raw -/// Parquet schema, where any node can carry an id, as Spark's `containsFieldIds` does. -/// -/// Dictionary and run-end-encoded wrappers are not walked, because the Parquet read path never -/// nests a struct, list or map inside them. -fn field_holds_id(field: &Field) -> bool { - field_id(field).is_some() - || match field.data_type() { - DataType::Struct(fields) => any_nested_field_has_id(fields), - DataType::Map(entries, _) => field_holds_id(entries), - other => list_element_field(other).is_some_and(|f| field_holds_id(f)), - } -} - -/// The element field of a list in any Arrow representation, or `None` for a type that is not a -/// list. -fn list_element_field(data_type: &DataType) -> Option<&FieldRef> { - match data_type { - DataType::List(f) - | DataType::LargeList(f) - | DataType::FixedSizeList(f, _) - | DataType::ListView(f) - | DataType::LargeListView(f) => Some(f), - _ => None, - } -} - /// Resolve each requested (`to`) struct field to the index of the file (`from`) field it reads /// from, or `None` when the file holds no such field. Mirrors Spark's `clipParquetGroupFields`: /// when the requested struct carries Parquet field IDs anywhere (and `use_field_id` is set), @@ -2128,58 +2070,4 @@ mod tests { "unexpected error: {err}" ); } - - /// The recursive id check sees an id on a root field, on a struct child, on the element of - /// every list representation, and on a map key or value. It sees none on a schema without - /// ids and none on an empty schema. - #[test] - fn any_nested_field_has_id_finds_ids_at_every_depth() { - use super::any_nested_field_has_id; - use arrow::datatypes::{DataType, Field, Fields}; - use parquet::arrow::PARQUET_FIELD_ID_META_KEY; - - let plain = |name: &str| Field::new(name, DataType::Int32, true); - let tagged = |name: &str| { - plain(name).with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "7".to_string(), - )])) - }; - let root = |field: Field| Fields::from(vec![field]); - let has_id = |field: Field| any_nested_field_has_id(&root(field)); - - assert!(!any_nested_field_has_id(&Fields::empty())); - assert!(!has_id(plain("x"))); - assert!(has_id(tagged("x"))); - - let strukt = |child: Field| Field::new("s", DataType::Struct(root(child)), true); - assert!(has_id(strukt(tagged("c")))); - assert!(!has_id(strukt(plain("c")))); - - let element = Arc::new(tagged("item")); - for list_type in [ - DataType::List(Arc::clone(&element)), - DataType::LargeList(Arc::clone(&element)), - DataType::FixedSizeList(Arc::clone(&element), 2), - DataType::ListView(Arc::clone(&element)), - DataType::LargeListView(Arc::clone(&element)), - ] { - let list = Field::new("l", list_type.clone(), true); - assert!(has_id(list), "{list_type}"); - } - let plain_list = Field::new("l", DataType::List(Arc::new(plain("item"))), true); - assert!(!has_id(plain_list)); - - let map = |key: Field, value: Field| { - let entries = Field::new( - "entries", - DataType::Struct(Fields::from(vec![key, value])), - false, - ); - Field::new("m", DataType::Map(Arc::new(entries), false), true) - }; - assert!(has_id(map(tagged("key"), plain("value")))); - assert!(has_id(map(plain("key"), tagged("value")))); - assert!(!has_id(map(plain("key"), plain("value")))); - } } diff --git a/native/core/src/parquet/schema_adapter.rs b/native/core/src/parquet/schema_adapter.rs index 590a560940e..36f1a9c749f 100644 --- a/native/core/src/parquet/schema_adapter.rs +++ b/native/core/src/parquet/schema_adapter.rs @@ -76,11 +76,9 @@ fn parse_field_id(field: &Field) -> Option { .and_then(|v| v.parse::().ok()) } -/// True when a root field of `schema` carries a Parquet field id. This stays root-only on -/// purpose: it gates the root name remap, and Spark's `clipParquetGroupFields` decides id -/// matching one struct level at a time. Whether a file holds ids at all is a different question, -/// answered by the reader factory over the raw Parquet schema before the adapter is built. -fn schema_has_field_ids(schema: &SchemaRef) -> bool { +/// True when a root field of `schema` carries a field id. Root only on purpose: it gates the +/// root name remap, and Spark's `clipParquetGroupFields` decides id matching one level at a time. +fn any_root_field_has_id(schema: &SchemaRef) -> bool { schema.fields().iter().any(|f| parse_field_id(f).is_some()) } @@ -208,9 +206,7 @@ fn remap_physical_schema( case_sensitive: bool, use_field_id: bool, ) -> DataFusionResult<(SchemaRef, HashMap)> { - // Root ids alone decide whether to match by id here. The check that the file holds ids at - // all runs earlier, in the reader factory, over the raw Parquet schema. - let should_match_by_id = use_field_id && schema_has_field_ids(logical_schema); + let should_match_by_id = use_field_id && any_root_field_has_id(logical_schema); // Build id -> all matching physical field names. We need the full list so we can mirror // Spark's `_LEGACY_ERROR_TEMP_2094` "Found duplicate field(s)" error when an ID-bearing @@ -887,15 +883,9 @@ impl PhysicalExprAdapterFactory for SparkPhysicalExprAdapterFactory { // to the original physical names. This is necessary because downstream code // (reassign_expr_columns) looks up columns by name in the actual stream schema, // which uses the original physical file column names. - // - // The check that a file carries field ids at all, which Spark's `ParquetReadSupport` - // runs before anything else, lives in `EagerPageIndexReader::get_metadata`, where the - // raw Parquet schema is at hand. By the time the schemas reach this point the INT96 - // coercion may have dropped the ids from container fields, so `physical_file_schema` - // cannot answer that question. let case_sensitive = self.parquet_options.case_sensitive; let should_match_by_id = - self.parquet_options.use_field_id && schema_has_field_ids(&logical_file_schema); + self.parquet_options.use_field_id && any_root_field_has_id(&logical_file_schema); let needs_remap = !case_sensitive || should_match_by_id; let (adapted_physical_schema, logical_to_physical_names) = if needs_remap { let (remapped, logical_to_physical) = remap_physical_schema( @@ -1635,8 +1625,7 @@ impl PhysicalExpr for RejectOnNonEmpty { #[cfg(test)] mod test { use crate::parquet::cast_column::CometCastColumnExpr; - use crate::parquet::parquet_exec::init_datasource_exec; - use crate::parquet::parquet_support::{ObjectStoreBackend, SparkParquetOptions}; + use crate::parquet::parquet_support::SparkParquetOptions; use crate::parquet::schema_adapter::{ check_conversion, is_pure_structural_narrowing, ConversionCheck, SparkPhysicalExprAdapterFactory, @@ -1664,18 +1653,12 @@ mod test { use datafusion::physical_expr::expressions::Column; use datafusion::physical_expr::PhysicalExpr; use datafusion::physical_plan::{ExecutionPlan, SendableRecordBatchStream}; - use datafusion::prelude::SessionContext; use datafusion_comet_spark_expr::test_common::file_util::get_temp_filename; use datafusion_comet_spark_expr::EvalMode; use datafusion_physical_expr_adapter::PhysicalExprAdapterFactory; use futures::StreamExt; - use parquet::arrow::arrow_writer::ArrowWriterOptions; use parquet::arrow::ArrowWriter; use parquet::arrow::PARQUET_FIELD_ID_META_KEY; - use parquet::basic::{LogicalType, Repetition, Type as PhysicalType}; - use parquet::data_type::{Int32Type as ParquetInt32Type, Int96, Int96Type}; - use parquet::file::writer::SerializedFileWriter; - use parquet::schema::types::Type as ParquetType; use parquet::variant::VariantType; use std::collections::HashMap; use std::fs::File; @@ -2968,595 +2951,6 @@ mod test { Ok(()) } - /// The message every rejected read carries, from `SparkError::ParquetMissingFieldIds`. - const MISSING_IDS: &str = "Parquet file schema doesn't contain any field Ids"; - - /// A fresh temporary file path as a `String`. - fn temp_filename() -> String { - let filename = get_temp_filename(); - filename.as_path().as_os_str().to_str().unwrap().to_string() - } - - /// Write `batch` to a temporary file and return its path. With `skip_arrow_metadata` the - /// file carries no Arrow schema hint, so the reader derives every field, ids included, from - /// the Parquet schema alone. - fn write_parquet( - batch: &RecordBatch, - skip_arrow_metadata: bool, - ) -> Result { - let filename = temp_filename(); - let file = File::create(&filename)?; - let options = ArrowWriterOptions::new().with_skip_arrow_metadata(skip_arrow_metadata); - let mut writer = ArrowWriter::try_new_with_options(file, batch.schema(), options)?; - writer.write(batch)?; - writer.close()?; - Ok(filename) - } - - /// A one-column batch `name: int32` holding 1 and 2, with `metadata` on the field. - fn int_batch(name: &str, metadata: HashMap) -> RecordBatch { - let schema = Arc::new(Schema::new(vec![ - Field::new(name, DataType::Int32, true).with_metadata(metadata) - ])); - RecordBatch::try_new( - schema, - vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef], - ) - .unwrap() - } - - /// What the planner hands `init_datasource_exec` that the field id tests vary. - struct PlannerScan { - required_schema: SchemaRef, - data_schema: SchemaRef, - projection: Vec, - use_field_id: bool, - ignore_missing_field_id: bool, - case_sensitive: bool, - } - - impl PlannerScan { - /// A scan that reads all of `required_schema`, as an unpruned query does. - fn of(required_schema: SchemaRef) -> Self { - Self { - data_schema: Arc::clone(&required_schema), - projection: (0..required_schema.fields().len()).collect(), - required_schema, - use_field_id: false, - ignore_missing_field_id: false, - case_sensitive: false, - } - } - } - - /// Scan `filename` the way the planner does, through `init_datasource_exec`, so the reader - /// factory and the schema adapter see exactly what a Spark query hands them: `data_schema` - /// is the full read schema, `required_schema` the pruned one and `projection` picks the - /// required columns out of `data_schema`. - fn scan_file_via_planner( - filename: String, - scan: PlannerScan, - ) -> Result { - let session_ctx = Arc::new(SessionContext::new()); - let exec = init_datasource_exec( - scan.required_schema, - Some(scan.data_schema), - None, - ObjectStoreUrl::local_filesystem(), - ObjectStoreBackend::Local, - vec![vec![PartitionedFile::from_path(filename)?]], - Some(scan.projection), - None, - None, - "UTC", - scan.case_sensitive, - true, - false, - false, - &session_ctx, - false, - scan.use_field_id, - scan.ignore_missing_field_id, - ) - .expect("planning the scan"); - exec.execute(0, session_ctx.task_ctx()) - } - - /// The message of the error a scan raises, either while planning or on its first poll. - async fn first_poll_error( - stream: Result, - ) -> String { - match stream { - Err(err) => err.to_string(), - Ok(mut stream) => stream - .next() - .await - .unwrap() - .expect_err("expected the scan to be rejected") - .to_string(), - } - } - - /// Batch `s: struct` with no id on the root field, so a file written from it - /// carries ids only below the root. - fn nested_id_batch() -> Result { - struct_batch( - Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")), - Arc::new(Int32Array::from(vec![1, 2])), - ) - } - - /// Spark checks for missing file ids before it decides whether to match by id, so the - /// rejection fires even when `fieldId.read.enabled` is off. A case-sensitive session keeps - /// the name remap out of the picture. - #[tokio::test] - async fn missing_file_field_ids_rejected_when_id_matching_disabled( - ) -> Result<(), DataFusionError> { - let batch = int_batch("a", HashMap::new()); - let required_schema = Arc::new(Schema::new(vec![ - Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) - ])); - let scan = PlannerScan { - case_sensitive: true, - ..PlannerScan::of(required_schema) - }; - let msg = - first_poll_error(scan_file_via_planner(write_parquet(&batch, false)?, scan)).await; - assert!(msg.contains(MISSING_IDS), "unexpected error: {msg}"); - Ok(()) - } - - /// A read schema whose only ids sit on struct children still expects ids, as Spark's - /// `ParquetUtils.hasFieldIds` walks nested fields. A file with no ids anywhere is rejected. - #[tokio::test] - async fn nested_logical_field_ids_rejected_when_file_has_none() -> Result<(), DataFusionError> { - let batch = struct_batch( - Field::new("a", DataType::Int32, true), - Arc::new(Int32Array::from(vec![1, 2])), - )?; - let required_schema = struct_schema(vec![ - Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")) - ]); - let scan = PlannerScan { - use_field_id: true, - ..PlannerScan::of(required_schema) - }; - let msg = - first_poll_error(scan_file_via_planner(write_parquet(&batch, false)?, scan)).await; - assert!(msg.contains(MISSING_IDS), "unexpected error: {msg}"); - Ok(()) - } - - /// Ids that only appear below the root of the file still count as ids, as in Spark's - /// `containsFieldIds`. The read passes the missing-id check, resolves the nested field by - /// id, and null-fills a root field whose id the file does not hold. The second round writes - /// the file without an Arrow schema hint, so the ids come from the Parquet schema alone. - #[tokio::test] - async fn nested_file_field_ids_satisfy_the_missing_id_check() -> Result<(), DataFusionError> { - let batch = nested_id_batch()?; - let required_schema = Arc::new(Schema::new(vec![ - Field::new( - "s", - DataType::Struct(Fields::from(vec![ - Field::new("a", DataType::Int32, true).with_metadata(id_meta("11")) - ])), - true, - ), - Field::new("missing", DataType::Int32, true).with_metadata(id_meta("7")), - ])); - for skip_arrow_metadata in [false, true] { - let scan = PlannerScan { - use_field_id: true, - ..PlannerScan::of(Arc::clone(&required_schema)) - }; - let mut stream = - scan_file_via_planner(write_parquet(&batch, skip_arrow_metadata)?, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!( - result.num_rows(), - 2, - "skip_arrow_metadata={skip_arrow_metadata}" - ); - let s = result.column(0).as_struct(); - assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); - assert_eq!(result.column(1).null_count(), 2); - } - Ok(()) - } - - /// With `ignoreMissing` set, a file without ids reads under an id-bearing schema and the - /// unmatched field is null-filled instead of raising. - #[tokio::test] - async fn missing_file_field_ids_allowed_when_ignore_missing_is_set( - ) -> Result<(), DataFusionError> { - let batch = int_batch("a", HashMap::new()); - let required_schema = Arc::new(Schema::new(vec![ - Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")) - ])); - let scan = PlannerScan { - use_field_id: true, - ignore_missing_field_id: true, - ..PlannerScan::of(required_schema) - }; - let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.num_rows(), 2); - assert_eq!(result.column(0).null_count(), 2); - Ok(()) - } - - /// With id matching off, ids on both sides play no part: the file is not rejected and the - /// read schema field still resolves by name. A name the file lacks reads as null even though - /// the file holds a field with the same id. - #[tokio::test] - async fn file_ids_ignored_when_id_matching_disabled() -> Result<(), DataFusionError> { - let batch = int_batch("a", id_meta("1")); - let required_schema = Arc::new(Schema::new(vec![ - Field::new("b", DataType::Int32, true).with_metadata(id_meta("1")) - ])); - let scan = PlannerScan::of(required_schema); - let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.num_rows(), 2); - assert_eq!(result.column(0).null_count(), 2); - Ok(()) - } - - /// A read schema without ids never triggers the missing-id check, whatever the flags say. - /// The file name differs only in case so the adapter is still created. - #[tokio::test] - async fn no_logical_field_ids_never_rejects() -> Result<(), DataFusionError> { - let batch = int_batch("A", HashMap::new()); - let required_schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)])); - let scan = PlannerScan { - use_field_id: true, - ..PlannerScan::of(required_schema) - }; - let mut stream = scan_file_via_planner(write_parquet(&batch, false)?, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!( - result.column(0).as_primitive::().values(), - &[1, 2] - ); - Ok(()) - } - - /// Spark checks for missing file ids against the pruned read schema, so an id on a column - /// the query never projects does not reject a file without ids, whatever the read flag - /// says. The same read with both columns projected still raises. - #[tokio::test] - async fn field_ids_on_an_unprojected_column_do_not_reject_a_file_without_ids( - ) -> Result<(), DataFusionError> { - let file_schema = Arc::new(Schema::new(vec![ - Field::new("a", DataType::Int32, true), - Field::new("b", DataType::Int32, true), - ])); - let batch = RecordBatch::try_new( - file_schema, - vec![ - Arc::new(Int32Array::from(vec![1, 3])) as ArrayRef, - Arc::new(Int32Array::from(vec![2, 4])) as ArrayRef, - ], - )?; - let filename = write_parquet(&batch, false)?; - let a = Field::new("a", DataType::Int32, true).with_metadata(id_meta("1")); - let b = Field::new("b", DataType::Int32, true); - let read_schema = Arc::new(Schema::new(vec![a, b.clone()])); - let pruned_schema = Arc::new(Schema::new(vec![b])); - - for use_field_id in [false, true] { - let scan = PlannerScan { - required_schema: Arc::clone(&pruned_schema), - projection: vec![1], - use_field_id, - ..PlannerScan::of(Arc::clone(&read_schema)) - }; - let mut stream = scan_file_via_planner(filename.clone(), scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.num_columns(), 1, "use_field_id={use_field_id}"); - assert_eq!( - result.column(0).as_primitive::().values(), - &[2, 4], - "use_field_id={use_field_id}" - ); - - let scan = PlannerScan { - use_field_id, - ..PlannerScan::of(Arc::clone(&read_schema)) - }; - let msg = first_poll_error(scan_file_via_planner(filename.clone(), scan)).await; - assert!( - msg.contains(MISSING_IDS), - "use_field_id={use_field_id}: unexpected error: {msg}" - ); - } - Ok(()) - } - - /// 2000-01-01T00:00:00Z as an INT96 value: no nanoseconds into Julian day 2451545. - fn int96_epoch_2000() -> Int96 { - let mut value = Int96::new(); - value.set_data(0, 0, 2_451_545); - value - } - - /// 2000-01-01T00:00:00Z in microseconds since the Unix epoch. - const EPOCH_2000_MICROS: i64 = 946_684_800_000_000; - - /// Write `message schema { required group s { required int32 a; required int96 ts; } }` with - /// rows (1, 2000-01-01) and (2, 2000-01-01) through the low level writer, since the Arrow - /// writer cannot produce INT96. `id` goes on the group `s` and nowhere else. - fn write_struct_with_int96(id: Option) -> Result { - let a = Arc::new( - ParquetType::primitive_type_builder("a", PhysicalType::INT32) - .with_repetition(Repetition::REQUIRED) - .build()?, - ); - let ts = Arc::new( - ParquetType::primitive_type_builder("ts", PhysicalType::INT96) - .with_repetition(Repetition::REQUIRED) - .build()?, - ); - let s = Arc::new( - ParquetType::group_type_builder("s") - .with_repetition(Repetition::REQUIRED) - .with_id(id) - .with_fields(vec![a, ts]) - .build()?, - ); - let schema = Arc::new( - ParquetType::group_type_builder("schema") - .with_fields(vec![s]) - .build()?, - ); - let filename = temp_filename(); - let file = File::create(&filename)?; - let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; - let mut row_group = writer.next_row_group()?; - let mut column = row_group.next_column()?.unwrap(); - column - .typed::() - .write_batch(&[1, 2], None, None)?; - column.close()?; - let mut column = row_group.next_column()?.unwrap(); - column.typed::().write_batch( - &[int96_epoch_2000(), int96_epoch_2000()], - None, - None, - )?; - column.close()?; - row_group.close()?; - writer.close()?; - Ok(filename) - } - - /// Read schema `s (id 1): struct`, the shape Spark requests for a - /// struct holding a timestamp column. - fn struct_with_timestamp_schema() -> SchemaRef { - Arc::new(Schema::new(vec![Field::new( - "s", - DataType::Struct(Fields::from(vec![ - Field::new("a", DataType::Int32, true), - Field::new( - "ts", - DataType::Timestamp(TimeUnit::Microsecond, Some("UTC".into())), - true, - ), - ])), - true, - ) - .with_metadata(id_meta("1"))])) - } - - /// A struct holding an INT96 timestamp reaches the schema adapter without its id, because - /// the INT96 coercion rebuilds every container field and copies no metadata. The missing-id - /// check reads the Parquet schema itself, where the id is, so the file is not rejected and - /// the struct resolves by name. Resolving it by id is a matter for the remap, which still - /// works from the coerced Arrow schema, so id matching stays off here. - #[tokio::test] - async fn struct_id_hidden_by_int96_coercion_satisfies_the_missing_id_check( - ) -> Result<(), DataFusionError> { - let filename = write_struct_with_int96(Some(1))?; - let scan = PlannerScan::of(struct_with_timestamp_schema()); - let mut stream = scan_file_via_planner(filename, scan)?; - let result = stream.next().await.unwrap()?; - let s = result.column(0).as_struct(); - assert_eq!(s.column(0).as_primitive::().values(), &[1, 2]); - assert_eq!( - s.column(1) - .as_primitive::() - .values(), - &[EPOCH_2000_MICROS, EPOCH_2000_MICROS] - ); - Ok(()) - } - - /// The same struct written without any id is still rejected under the id-bearing read - /// schema, so the check reads the Parquet schema rather than passing every INT96 file. - #[tokio::test] - async fn struct_with_int96_and_no_ids_is_rejected() -> Result<(), DataFusionError> { - let filename = write_struct_with_int96(None)?; - for use_field_id in [false, true] { - let scan = PlannerScan { - use_field_id, - ..PlannerScan::of(struct_with_timestamp_schema()) - }; - let msg = first_poll_error(scan_file_via_planner(filename.clone(), scan)).await; - assert!( - msg.contains(MISSING_IDS), - "use_field_id={use_field_id}: unexpected error: {msg}" - ); - } - Ok(()) - } - - /// Write `message schema { optional group l (LIST) { repeated group list { required int32 - /// element; } } }` with rows [1] and [2]. The only id, 5, sits on the repeated `list` - /// group, which the Arrow schema never shows. - fn write_list_with_id_on_list_group() -> Result { - let element = Arc::new( - ParquetType::primitive_type_builder("element", PhysicalType::INT32) - .with_repetition(Repetition::REQUIRED) - .build()?, - ); - let list = Arc::new( - ParquetType::group_type_builder("list") - .with_repetition(Repetition::REPEATED) - .with_id(Some(5)) - .with_fields(vec![element]) - .build()?, - ); - let l = Arc::new( - ParquetType::group_type_builder("l") - .with_repetition(Repetition::OPTIONAL) - .with_logical_type(Some(LogicalType::List)) - .with_fields(vec![list]) - .build()?, - ); - let schema = Arc::new( - ParquetType::group_type_builder("schema") - .with_fields(vec![l]) - .build()?, - ); - let filename = temp_filename(); - let file = File::create(&filename)?; - let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; - let mut row_group = writer.next_row_group()?; - let mut column = row_group.next_column()?.unwrap(); - column - .typed::() - .write_batch(&[1, 2], Some(&[2, 2]), Some(&[0, 0]))?; - column.close()?; - row_group.close()?; - writer.close()?; - Ok(filename) - } - - /// Spark's `containsFieldIds` walks the raw Parquet schema, where an id may sit on the - /// repeated `list` group that the Arrow schema folds away. Such a file is not rejected. With - /// id matching off the list resolves by name. With it on, the root field asks for id 5, no - /// root field of the file carries it, and the column is null filled, as Spark does. - #[tokio::test] - async fn id_only_on_the_list_group_satisfies_the_missing_id_check( - ) -> Result<(), DataFusionError> { - let filename = write_list_with_id_on_list_group()?; - let read_schema = Arc::new(Schema::new(vec![Field::new( - "l", - DataType::List(Arc::new(Field::new("element", DataType::Int32, true))), - true, - ) - .with_metadata(id_meta("5"))])); - - let scan = PlannerScan::of(Arc::clone(&read_schema)); - let mut stream = scan_file_via_planner(filename.clone(), scan)?; - let result = stream.next().await.unwrap()?; - let l = result.column(0).as_list::(); - assert_eq!(l.len(), 2); - assert_eq!(l.values().as_primitive::().values(), &[1, 2]); - - let scan = PlannerScan { - use_field_id: true, - ..PlannerScan::of(read_schema) - }; - let mut stream = scan_file_via_planner(filename, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.column(0).null_count(), 2); - Ok(()) - } - - /// Write `message schema { optional group m (MAP) { repeated group key_value { required - /// int32 key; optional int32 value; } } }` with rows {1: 10} and {2: 20}. The only id, 5, - /// sits on the repeated `key_value` group, which the Arrow schema never shows. - fn write_map_with_id_on_key_value_group() -> Result { - let key = Arc::new( - ParquetType::primitive_type_builder("key", PhysicalType::INT32) - .with_repetition(Repetition::REQUIRED) - .build()?, - ); - let value = Arc::new( - ParquetType::primitive_type_builder("value", PhysicalType::INT32) - .with_repetition(Repetition::OPTIONAL) - .build()?, - ); - let key_value = Arc::new( - ParquetType::group_type_builder("key_value") - .with_repetition(Repetition::REPEATED) - .with_id(Some(5)) - .with_fields(vec![key, value]) - .build()?, - ); - let m = Arc::new( - ParquetType::group_type_builder("m") - .with_repetition(Repetition::OPTIONAL) - .with_logical_type(Some(LogicalType::Map)) - .with_fields(vec![key_value]) - .build()?, - ); - let schema = Arc::new( - ParquetType::group_type_builder("schema") - .with_fields(vec![m]) - .build()?, - ); - let filename = temp_filename(); - let file = File::create(&filename)?; - let mut writer = SerializedFileWriter::new(file, schema, Default::default())?; - let mut row_group = writer.next_row_group()?; - let mut column = row_group.next_column()?.unwrap(); - column - .typed::() - .write_batch(&[1, 2], Some(&[2, 2]), Some(&[0, 0]))?; - column.close()?; - let mut column = row_group.next_column()?.unwrap(); - column - .typed::() - .write_batch(&[10, 20], Some(&[3, 3]), Some(&[0, 0]))?; - column.close()?; - row_group.close()?; - writer.close()?; - Ok(filename) - } - - /// The map counterpart of the list case: an id on the repeated `key_value` group, which the - /// Arrow schema folds away, still counts for Spark's `containsFieldIds`. With id matching - /// off the map resolves by name. With it on, the root field asks for id 5, no root field of - /// the file carries it, and the column is null filled, as Spark does. - #[tokio::test] - async fn id_only_on_the_key_value_group_satisfies_the_missing_id_check( - ) -> Result<(), DataFusionError> { - let filename = write_map_with_id_on_key_value_group()?; - let entries = Field::new( - "key_value", - DataType::Struct(Fields::from(vec![ - Field::new("key", DataType::Int32, false), - Field::new("value", DataType::Int32, true), - ])), - false, - ); - let read_schema = Arc::new(Schema::new(vec![Field::new( - "m", - DataType::Map(Arc::new(entries), false), - true, - ) - .with_metadata(id_meta("5"))])); - - let scan = PlannerScan::of(Arc::clone(&read_schema)); - let mut stream = scan_file_via_planner(filename.clone(), scan)?; - let result = stream.next().await.unwrap()?; - let m = result.column(0).as_map(); - assert_eq!(m.len(), 2); - assert_eq!(m.keys().as_primitive::().values(), &[1, 2]); - assert_eq!(m.values().as_primitive::().values(), &[10, 20]); - - let scan = PlannerScan { - use_field_id: true, - ..PlannerScan::of(read_schema) - }; - let mut stream = scan_file_via_planner(filename, scan)?; - let result = stream.next().await.unwrap()?; - assert_eq!(result.column(0).null_count(), 2); - Ok(()) - } - /// Disallowed widening (`INT32 -> bigint` with `allow_type_promotion` off) inside a /// struct defers to `RejectOnNonEmpty`, like the top level: a non-empty file fails ... #[tokio::test] diff --git a/native/jni-bridge/src/errors.rs b/native/jni-bridge/src/errors.rs index 6cc9299c3f1..199ad94246d 100644 --- a/native/jni-bridge/src/errors.rs +++ b/native/jni-bridge/src/errors.rs @@ -648,8 +648,7 @@ fn throw_spark_error_as_json(env: &mut Env, spark_error: &SparkError) -> jni::er ) } -/// A `SparkError` raised by the Parquet reader while opening a file, such as the missing field -/// id check in the reader factory's `get_metadata`, arrives as +/// A `SparkError` the Parquet reader raised on open arrives as /// `DataFusionError::ParquetError(ParquetError::External(spark_error))`. Unwrap it so the error /// keeps its own JVM exception class instead of being classified as a file read failure. /// `Context` and `Shared` wrappers are looked through, as `try_classify_file_read_error` does. @@ -1396,21 +1395,23 @@ mod tests { #[test] fn parquet_external_spark_error_keeps_its_type() { let raised = DataFusionError::ParquetError(Box::new(ParquetError::External(Box::new( - SparkError::ParquetMissingFieldIds, + SparkError::ParquetMissingFieldIds { + file_path: "a.parquet".to_string(), + }, )))); assert!(matches!( parquet_external_spark_error(&raised), - Some(SparkError::ParquetMissingFieldIds) + Some(SparkError::ParquetMissingFieldIds { file_path }) if file_path == "a.parquet" )); let wrapped = DataFusionError::Context("open".to_string(), Box::new(raised)); assert!(matches!( parquet_external_spark_error(&wrapped), - Some(SparkError::ParquetMissingFieldIds) + Some(SparkError::ParquetMissingFieldIds { .. }) )); let shared = DataFusionError::Shared(Arc::new(wrapped)); assert!(matches!( parquet_external_spark_error(&shared), - Some(SparkError::ParquetMissingFieldIds) + Some(SparkError::ParquetMissingFieldIds { .. }) )); let corrupt = DataFusionError::ParquetError(Box::new(ParquetError::General( "corrupt footer".to_string(), diff --git a/native/proto/src/proto/operator.proto b/native/proto/src/proto/operator.proto index 6a6284fb4c1..7b3a7aec0d2 100644 --- a/native/proto/src/proto/operator.proto +++ b/native/proto/src/proto/operator.proto @@ -163,8 +163,11 @@ message NativeScanCommon { // schema actually carries parquet.field.id metadata. When false the native // scan keeps its existing name-based path with no extra work. bool use_field_id = 15; - // True when spark.sql.parquet.fieldId.read.ignoreMissing is set. - bool ignore_missing_field_id = 16; + // True when the requested schema carries a field id and + // spark.sql.parquet.fieldId.read.ignoreMissing is not set. The native scan + // then refuses a file whose Parquet schema carries no field id, as Spark's + // ParquetReadSupport does whether or not the read flag is set. + bool require_field_ids = 16; // Whether widening type promotion is allowed (e.g. INT32 -> INT64, // FLOAT -> DOUBLE). Set from Comet's per-Spark-version constant in // ShimCometConf (false on 3.x, true on 4.x). When false, reading a column 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 52c6959cdc6..a16d163ecf0 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 @@ -261,12 +261,13 @@ object CometNativeScan extends CometOperatorSerde[CometScanExec] with CometTypeS // Field-ID matching: only ask the native side to do extra work when the conf is on AND // the requested schema actually carries IDs. Spark's ParquetReadSupport applies the same // gate before invoking matchIdField. - val useFieldId = - scan.conf.getConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED) && - ParquetUtils.hasFieldIds(scan.requiredSchema) + val hasFieldIds = ParquetUtils.hasFieldIds(scan.requiredSchema) + val useFieldId = scan.conf.getConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED) && hasFieldIds commonBuilder.setUseFieldId(useFieldId) - commonBuilder.setIgnoreMissingFieldId( - scan.conf.getConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID)) + // Spark's ParquetReadSupport refuses a file without field ids whenever the requested + // schema carries one, whatever the read flag says, unless ignoreMissing is set. + commonBuilder.setRequireFieldIds( + hasFieldIds && !scan.conf.getConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID)) commonBuilder.setAllowTypePromotion(CometConf.COMET_SCHEMA_EVOLUTION_ENABLED) commonBuilder.setAllowTimestampLtzToNtz(CometConf.COMET_ALLOW_TIMESTAMP_LTZ_AS_NTZ) 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 ec9aa00b03c..3c8a9b26564 100644 --- a/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala @@ -2114,76 +2114,53 @@ abstract class ParquetReadSuite extends CometTestBase { } } - // Verbatim port of Spark `ParquetFieldIdIOSuite.test("read parquet file without ids")`, - // for the same reason as the duplicate-id test above. + // Port of Spark `ParquetFieldIdIOSuite.test("read parquet file without ids")`, for the same + // reason as the duplicate-id test above. It runs with the read flag off as well, since Spark's + // `ParquetReadSupport` checks for missing ids before it consults the flag, and with id zero as + // one of the read schemas, since zero is an id like any other. test("read parquet file without ids") { - withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "true") { - withTempPath { dir => - val readSchema = - new StructType() - .add("a", IntegerType, true, withId(1)) - - val writeSchema = - new StructType() - .add("a", IntegerType, true) - .add("rand1", StringType, true) - .add("rand2", StringType, true) - - val writeData = Seq(Row(100, "text", "txt"), Row(200, "more", "mr")) - spark - .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) + Seq("true", "false").foreach { readEnabled => + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { + withTempPath { dir => + val readSchema = + new StructType() + .add("a", IntegerType, true, withId(1)) - Seq(readSchema, readSchema.add("b", StringType, true)).foreach { schema => - val cause = intercept[SparkException] { - spark.read.schema(schema).parquet(dir.getCanonicalPath).collect() - }.getCause - assert( - cause.isInstanceOf[RuntimeException] && - cause.getMessage.contains("Parquet file schema doesn't contain any field Ids")) - val expectedValues = (1 to schema.length).map(_ => null) - withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { - checkAnswer( - spark.read.schema(schema).parquet(dir.getCanonicalPath), - Row(expectedValues: _*) :: Row(expectedValues: _*) :: Nil) - } - } - } - } - } + val writeSchema = + new StructType() + .add("a", IntegerType, true) + .add("rand1", StringType, true) + .add("rand2", StringType, true) - // Spark's `ParquetReadSupport` checks for missing file ids before it looks at - // `fieldId.read.enabled`, so the error is raised with id matching off as well. With - // `ignoreMissing` set, both engines fall back to matching by name and read real values. - test("read schema with field ids raises on a file without ids when id matching is off") { - withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { - withTempPath { dir => - val readSchema = new StructType().add("a", IntegerType, true, withId(1)) - val writeSchema = new StructType().add("a", IntegerType, true) - val writeData = Seq(Row(100), Row(200)) - spark - .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) + val writeData = Seq(Row(100, "text", "txt"), Row(200, "more", "mr")) + spark + .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) + .write + .mode("overwrite") + .parquet(dir.getCanonicalPath) - def readCause(): Throwable = intercept[SparkException] { - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() - }.getCause - withClue("Spark with Comet disabled") { - withSQLConf(CometConf.COMET_ENABLED.key -> "false") { - assertMissingIdsException(readCause()) + val idZeroSchema = new StructType().add("a", IntegerType, true, withId(0)) + Seq(readSchema, readSchema.add("b", StringType, true), idZeroSchema).foreach { schema => + withClue(s"read flag $readEnabled, schema $schema: ") { + val cause = intercept[SparkException] { + spark.read.schema(schema).parquet(dir.getCanonicalPath).collect() + }.getCause + assert(cause.isInstanceOf[RuntimeException] && + cause.getMessage.contains("Parquet file schema doesn't contain any field Ids")) + withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { + val df = spark.read.schema(schema).parquet(dir.getCanonicalPath) + if (readEnabled.toBoolean) { + // Spark's own assertion: no file field carries a requested id, so every + // column is null filled. + val expectedValues = (1 to schema.length).map(_ => null) + checkAnswer(df, Row(expectedValues: _*) :: Row(expectedValues: _*) :: Nil) + } else { + checkSparkAnswerAndOperator(df) + } + } + } } } - withClue("Comet") { - assertMissingIdsException(readCause()) - } - - withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { - checkSparkAnswerAndOperator(spark.read.schema(readSchema).parquet(dir.getCanonicalPath)) - } } } } @@ -2210,77 +2187,88 @@ abstract class ParquetReadSuite extends CometTestBase { } } - // Second half of Spark `ParquetFieldIdIOSuite.test("global read/write flag should work - // correctly")`: the file carries ids but the read flag is off, so columns resolve by name - // only. None of the read names exist in the file, so every value is null and nothing raises. - test("field ids in the file are ignored when id matching is off") { - withSQLConf( - SQLConf.PARQUET_FIELD_ID_WRITE_ENABLED.key -> "true", - SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { + // Spark's `containsFieldIds` walks the raw Parquet schema, where an id may sit on the + // repeated `list` or `key_value` group of a list or map, which no Spark or Arrow field ever + // shows. Such a file carries ids, so it is not rejected. With the read flag on, the root + // fields ask for ids that no root field of the file carries and are null filled. + test("ids on repeated list and key_value groups count as file ids") { + withTempDir { dir => + val path = new Path(dir.toURI.toString, "part-r-0.parquet") + val schema = MessageTypeParser.parseMessageType(""" + |message schema { + | optional group l (LIST) { + | repeated group list = 5 { + | optional int32 element; + | } + | } + | optional group m (MAP) { + | repeated group key_value = 6 { + | required int32 key; + | optional int32 value; + | } + | } + |} + |""".stripMargin) + val writer = createParquetWriter(schema, path) + (1 to 2).foreach { i => + val record = new SimpleGroup(schema) + record.addGroup(0).addGroup(0).add(0, i) + val entry = record.addGroup(1).addGroup(0) + entry.add(0, i) + entry.add(1, i * 10) + writer.write(record) + } + writer.close() + + val readSchema = new StructType() + .add("l", ArrayType(IntegerType), true, withId(5)) + .add("m", MapType(IntegerType, IntegerType), true, withId(6)) + Seq("false", "true").foreach { readEnabled => + withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { + withClue(s"read flag $readEnabled: ") { + checkSparkAnswerAndOperator( + spark.read.schema(readSchema).parquet(dir.getCanonicalPath)) + } + } + } + } + } + + // Spark checks for missing ids against the pruned schema it hands the reader, so an id on a + // column or struct child the query never reads does not reject a file without ids, and a + // count reads no column at all. A read that touches an id-bearing field still raises. + test("missing ids are checked against the pruned read schema") { + withSQLConf(SQLConf.NESTED_SCHEMA_PRUNING_ENABLED.key -> "true") { withTempPath { dir => - val readSchema = new StructType() - .add("some", IntegerType, true, withId(1)) - .add("other", StringType, true, withId(2)) - .add("name", StringType, true, withId(3)) val writeSchema = new StructType() + .add("a", IntegerType) + .add("b", IntegerType) + .add("s", new StructType().add("a", IntegerType).add("b", IntegerType)) + val readSchema = new StructType() .add("a", IntegerType, true, withId(1)) - .add("rand1", StringType, true, withId(2)) - .add("rand2", StringType, true, withId(3)) - val writeData = Seq(Row(100, "text", "txt"), Row(200, "more", "mr")) + .add("b", IntegerType) + .add( + "s", + new StructType().add("a", IntegerType, true, withId(11)).add("b", IntegerType)) + val writeData = Seq(Row(1, 2, Row(3, 4)), Row(5, 6, Row(7, 8))) spark .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) .write .mode("overwrite") .parquet(dir.getCanonicalPath) - val df = spark.read.schema(readSchema).parquet(dir.getCanonicalPath) - checkSparkAnswerAndOperator(df) - checkAnswer(df, Row(null, null, null) :: Row(null, null, null) :: Nil) - } - } - } - - // Spark checks for missing file ids against the pruned read schema, so an id on a column - // the query never projects does not reject a file without ids, whatever the read flag says. - // The same read with both columns projected still raises. - test("field ids on an unprojected column do not reject a file without ids") { - Seq("false", "true").foreach { readEnabled => - withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { - withTempPath { dir => - val writeSchema = new StructType().add("a", IntegerType).add("b", IntegerType) - val readSchema = new StructType() - .add("a", IntegerType, true, withId(1)) - .add("b", IntegerType, true) - val writeData = Seq(Row(1, 2), Row(3, 4)) - spark - .createDataFrame(spark.sparkContext.parallelize(writeData), writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) - - withClue(s"read flag $readEnabled") { - val pruned = spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("b") - checkSparkAnswerAndOperator(pruned) - checkAnswer(pruned, Row(2) :: Row(4) :: Nil) - - val cause = intercept[SparkException] { - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() - }.getCause - assert( - cause.isInstanceOf[RuntimeException] && - cause.getMessage.contains("Parquet file schema doesn't contain any field Ids"), - cause) - } - } + def read(): DataFrame = spark.read.schema(readSchema).parquet(dir.getCanonicalPath) + checkSparkAnswerAndOperator(read().select("b")) + checkSparkAnswerAndOperator(read().select("s.b")) + checkSparkAnswerAndOperator(read().selectExpr("count(*)")) + assertMissingFieldIds(read().select("a")) + assertMissingFieldIds(read().select("s.a")) } } } - // Spark writes timestamps as INT96 by default. The reader coerces INT96 to microseconds and - // rebuilds every container field without its metadata on the way, so the id on `s` is gone - // from the Arrow schema the adapter sees. The missing-id check reads the Parquet schema, where - // the id still is, so the file reads and `s` resolves by name. Resolving `s` by id is a - // matter for the remap, which still works from the coerced schema, so id matching stays off. + // Spark writes timestamps as INT96 by default. The id on `s` is in the Parquet schema, which + // the check reads, so the file opens and `s` resolves by name. test("field ids on a struct holding a timestamp survive the missing-id check") { withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> "false") { withTempPath { dir => @@ -2294,9 +2282,7 @@ abstract class ParquetReadSuite extends CometTestBase { .mode("overwrite") .parquet(dir.getCanonicalPath) - val df = spark.read.schema(schema).parquet(dir.getCanonicalPath) - checkSparkAnswerAndOperator(df) - checkAnswer(df, Row(Row(1, ts)) :: Row(Row(2, ts)) :: Nil) + checkSparkAnswerAndOperator(spark.read.schema(schema).parquet(dir.getCanonicalPath)) } } } @@ -2321,10 +2307,7 @@ abstract class ParquetReadSuite extends CometTestBase { .mode("append") .parquet(dir.getCanonicalPath) - val cause = intercept[SparkException] { - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() - }.getCause - assertMissingIdsException(cause) + assertMissingFieldIds(spark.read.schema(readSchema).parquet(dir.getCanonicalPath)) withSQLConf(SQLConf.IGNORE_MISSING_PARQUET_FIELD_ID.key -> "true") { val df = spark.read.schema(readSchema).parquet(dir.getCanonicalPath) @@ -2335,112 +2318,24 @@ abstract class ParquetReadSuite extends CometTestBase { } } - // Nested schema pruning hands the reader only the struct children a query touches, so an id - // on `s.a` rejects a file without ids when `s.a` is read and plays no part when only `s.b` is. - test("field ids on a pruned struct child are checked only when that child is read") { - Seq("false", "true").foreach { readEnabled => - withSQLConf( - SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled, - SQLConf.NESTED_SCHEMA_PRUNING_ENABLED.key -> "true") { - withTempPath { dir => - val writeSchema = new StructType() - .add("s", new StructType().add("a", IntegerType).add("b", IntegerType), true) - val readSchema = new StructType().add( - "s", - StructType( - Seq( - StructField("a", IntegerType, nullable = true, withId(11)), - StructField("b", IntegerType, nullable = true))), - true) - spark - .createDataFrame( - spark.sparkContext.parallelize(Seq(Row(Row(1, 2)), Row(Row(3, 4)))), - writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) - - withClue(s"read flag $readEnabled") { - val cause = intercept[SparkException] { - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("s.a").collect() - }.getCause - assertMissingIdsException(cause) - - val onlyB = spark.read.schema(readSchema).parquet(dir.getCanonicalPath).select("s.b") - checkSparkAnswerAndOperator(onlyB) - checkAnswer(onlyB, Row(2) :: Row(4) :: Nil) - } - } - } - } + // Spark's own assertion from `ParquetFieldIdIOSuite`: the `SparkException` a read raises has + // the `RuntimeException` from `ParquetReadSupport` as its cause. + private def isMissingFieldIdsError(error: Throwable): Boolean = { + val cause = error.getCause + cause.isInstanceOf[RuntimeException] && + cause.getMessage.contains("Parquet file schema doesn't contain any field Ids") } - // Id zero is an id like any other, so a read schema carrying it expects ids in the file. - test("field id zero on a root field expects ids in the file") { - Seq("false", "true").foreach { readEnabled => - withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { - withTempPath { dir => - val readSchema = new StructType().add("a", IntegerType, true, withId(0)) - val writeSchema = new StructType().add("a", IntegerType, true) - spark - .createDataFrame(spark.sparkContext.parallelize(Seq(Row(1), Row(2))), writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) - - withClue(s"read flag $readEnabled") { - def readCause(): Throwable = intercept[SparkException] { - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).collect() - }.getCause - withSQLConf(CometConf.COMET_ENABLED.key -> "false") { - assertMissingIdsException(readCause()) - } - assertMissingIdsException(readCause()) - } - } - } - } - } - - // A count reads no columns, so the pruned read schema carries no ids and a file without ids - // is counted rather than rejected. - test("count over an id-bearing schema reads a file without ids") { - Seq("false", "true").foreach { readEnabled => - withSQLConf(SQLConf.PARQUET_FIELD_ID_READ_ENABLED.key -> readEnabled) { - withTempPath { dir => - val readSchema = new StructType().add("a", IntegerType, true, withId(1)) - val writeSchema = new StructType().add("a", IntegerType, true) - spark - .createDataFrame(spark.sparkContext.parallelize(Seq(Row(1), Row(2))), writeSchema) - .write - .mode("overwrite") - .parquet(dir.getCanonicalPath) - - withClue(s"read flag $readEnabled") { - val counted = - spark.read.schema(readSchema).parquet(dir.getCanonicalPath).selectExpr("count(*)") - checkSparkAnswerAndOperator(counted) - checkAnswer(counted, Row(2L) :: Nil) - } - } - } - } - } - - // Spark raises a plain `RuntimeException` for a read schema with ids over a file without any. - // How many `SparkException` layers sit above it varies with the Spark version. Spark 4 wraps - // a reader failure in a `FAILED_READ_FILE` `SparkException`, and on 4.0 and 4.1 that wrapper - // is the exception `collect()` raises, so the `RuntimeException` is its direct cause. The - // Comet shim follows Spark, so walk the cause chain instead of counting the layers above it. - private def assertMissingIdsException(cause: Throwable): Unit = { - val chain = Iterator.iterate(cause)(_.getCause).takeWhile(_ != null).toSeq - val missingIds = chain.find { t => - t.getClass == classOf[RuntimeException] && - Option(t.getMessage).exists(_.contains("Parquet file schema doesn't contain any field Ids")) + // Spark and Comet both raise the missing field ids error for `df`, and Comet plans the read + // natively, so the error comes from the native check rather than from a fallback to Spark. + private def assertMissingFieldIds(df: => DataFrame): Unit = { + checkCometOperators(stripAQEPlan(df.queryExecution.executedPlan)) + val (sparkError, cometError) = checkSparkAnswerMaybeThrows(df) + Seq("Spark" -> sparkError, "Comet" -> cometError).foreach { case (engine, error) => + assert( + error.exists(isMissingFieldIdsError), + s"$engine: " + error.map(causeChain(_).mkString("\n ")).getOrElse("no error")) } - assert( - missingIds.isDefined, - chain.map(t => s"${t.getClass.getName}: ${t.getMessage}").mkString("\n")) } }