sunchao commented on code in PR #6004: URL: https://github.com/apache/datafusion-comet/pull/6004#discussion_r4040747607
########## spark/src/test/scala/org/apache/comet/DataTypeSupportSuite.scala: ########## @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet + +import org.scalatest.funsuite.AnyFunSuite + +import org.apache.spark.sql.types._ + +import org.apache.comet.DataTypeSupport.{findDuplicateStructFieldIds, findDuplicateStructFieldNames} + +/** + * Unit tests for the recursive struct checks shared by the scan rule, the operator conversion + * gate and the Arrow cache serializer. Each of those reaches the same two questions -- can Java + * Arrow tell these struct children apart, and can Spark's Parquet reader resolve them one-to-one + * -- so the answers live in one place and are pinned here. + */ +class DataTypeSupportSuite extends AnyFunSuite { Review Comment: ### Correctness [P2] Register the new suite in both CI workflow matrices `dev/ci/check-suites.py` requires every non-excluded suite to appear in both `.github/workflows/pr_build_linux.yml` and `.github/workflows/pr_build_macos.yml`. Neither contains `org.apache.comet.DataTypeSupportSuite`. The [current preflight](https://github.com/apache/datafusion-comet/actions/runs/35263089768/job/105343403848) exits 255 with `Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.DataTypeSupportSuite`, which I also reproduced locally. This stops CI before the runtime test jobs run. Please add the suite to both matrices so the guard succeeds and the new unit tests execute. ########## spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala: ########## @@ -1098,6 +1099,16 @@ case class CometScanTypeChecker() extends DataTypeSupport with CometTypeShim { dt: DataType, name: String, fallbackReasons: ListBuffer[String]): Boolean = { + // Under field id matching Spark resolves each requested field to the one Parquet field + // carrying its id, and raises when more than one answers. A requested struct that repeats an + // id cannot be resolved that way, and the native scan reads it positionally rather than + // raising, so hand the read back to Spark and let it report the ambiguity. See #5801. + lazy val duplicateFieldIds = + if (CometParquetUtils.readFieldId(SQLConf.get)) { + DataTypeSupport.findDuplicateStructFieldIds(dt, name) Review Comment: ### Correctness [P2] Check duplicate field IDs on the complete requested schema `DataTypeSupport.isSchemaSupported` calls this method with each field's `dataType`, so `StructType(x: Long id=1, y: Long id=1)` reaches this helper twice as `LongType` and is accepted. I confirmed that with an isolated compile of the exact checker. Wrapping the same fields inside `s` correctly falls back. For a metadata-free Parquet file with those two top-level fields and the same requested schema, this leaves the same no-predicate bypass as the new nested fixture: [DataFusion 55.1 skips the expression adapter when the schemas are equal](https://github.com/apache/datafusion/blob/7d3835c71f30cbd3c3ae4041732267f1f453097a/datafusion/datasource-parquet/src/opener/mod.rs#L1010-L1042). Spark's field-ID ambiguity check applies at the root as well. Please run this check once on the complete requested schema and add the flattened version of the new reader fixture. The exception for repeated top-level output names does not apply to Parquet field IDs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
