ziting-openai commented on code in PR #5262:
URL: https://github.com/apache/datafusion-comet/pull/5262#discussion_r3935733976
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -1082,6 +1082,19 @@ object CometIcebergNativeScan extends
CometOperatorSerde[CometBatchScanExec] wit
}
}
+ // iceberg-rust validates a FileScanTask by resolving its
partition spec against
+ // the task schema, so a task carrying a partition spec needs
that spec's source
+ // columns present even when the query projects them out (e.g.
selecting only
+ // _spec_id / _partition). Union them in. project_field_ids
still drives the read,
+ // so these columns are not materialized into the output.
+ val schema: AnyRef =
+ IcebergReflection
+ .schemaWithRequiredFields(
+ baseSchema,
+ metadata.table,
+ IcebergReflection.partitionSourceFieldIds(task,
fileScanTaskClass))
Review Comment:
[P2] Preserve valid schemas when adding partition source fields
This new call can construct an invalid schema in two supported cases. First,
for `s:struct<region:string>` partitioned by `identity(s.region)`, the full
table schema already contains the nested source ID, but
`schemaWithRequiredFields` checks only top-level IDs through
`buildFieldIdMapping`. It appends the same field at the top level, so Iceberg's
`Schema` constructor rejects the duplicate ID. Even an ordinary `SELECT id`
without deletes reaches this path. The [Iceberg Spark CI
job](https://github.com/apache/datafusion-comet/actions/runs/33891332915/job/101092878196)
now reproduces this in nine parameterized cases across
`testNestedPartitionValues`, `testReadPartitionColumn`, and
`testPartitionedByNestedString`, including `Multiple entries with same key:
2=struct.string and 2=string`.
Second, [current Iceberg 1.11
CI](https://github.com/apache/datafusion-comet/actions/runs/33891332915/job/101092878262)
fails `testReaddColumnAfterIdentityPartitionDrop` at this call with `Invalid
schema: multiple fields for name category: 4 and 2`. The v1 test drops the
category partition field and column, re-adds category under a new ID, inserts a
row, then selects it. The original task spec still references the old void
source ID 2; this union restores its historical field alongside current
category ID 4. The partition serializer already omits the UNKNOWN value and
sends an empty spec for this case, so requiring that discarded source
introduces the collision.
Please derive required sources from the partition representation actually
sent to native, and merge fields with recursive ID checks while preserving
valid names and nesting. These failures propagate from execution-time
`serializePartitions`, without fallback. Both classes of collision were
observed on synthetic merge `282cc8faa482c4254a3173cfd7087e1130128712`, whose
relevant Comet files match this head. No local tests were run.
--
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]
