andygrove commented on code in PR #7028:
URL: https://github.com/apache/datafusion-comet/pull/7028#discussion_r4395383821
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -850,18 +851,17 @@ case class CometScanRule(session: SparkSession)
} else {
resolvedProjectedFieldIds
}
- val changed = schemas
- .flatMap { schema =>
- IcebergReflection.nestedFieldsAddedOrRenamed(metadata.table,
schema, fieldIds)
- }
+ val disjoint = metadata.taskSchema.toSeq
Review Comment:
You're right that this changes behavior. For files with neither field ids
nor a name mapping, iceberg-rust now matches a struct's children by position
only when the counts are equal, so a pruned read of such a struct fails with
"cannot reconcile struct fields by id". Before the bump it returned the file's
values. I don't think we can keep the old fallback for this case. Nothing at
planning time tells us whether a file has ids, and the old guard only fired
when the schema history showed a nested add or rename, so keeping it would put
back most of what this PR removes. Spark reads every nested field of these
files as NULL, so the native scan already differs there. That's tracked in
#6794 and listed under "Reads that differ from Spark" in iceberg.md, and the
upstream side is apache/iceberg-rust#3436. Does it work for you to leave this
as a documented divergence?
##########
native/Cargo.toml:
##########
@@ -85,8 +85,8 @@ aws-sdk-sts = { version = "1.114.0", default-features = false
}
# Pinned to a revision rather than a release because no published iceberg-rust
# version is on our arrow: 0.10.1 requires arrow/parquet ^58. Move to a plain
# version requirement once a release ships on arrow 59. Bump policy: #5645.
-iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"1f3bc34058e3f3e45336bc5428f37b92649a608b" }
-iceberg-storage-opendal = { git = "https://github.com/apache/iceberg-rust";,
rev = "1f3bc34058e3f3e45336bc5428f37b92649a608b", features = ["opendal-memory",
"opendal-fs", "opendal-s3", "opendal-gcs", "opendal-oss", "opendal-azdls"] }
+iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"b154603a710d730d2f43f8af4db257cc137237ef" }
+iceberg-storage-opendal = { git = "https://github.com/apache/iceberg-rust";,
rev = "b154603a710d730d2f43f8af4db257cc137237ef", features = ["opendal-memory",
"opendal-fs", "opendal-s3", "opendal-gcs", "opendal-oss", "opendal-azdls"] }
Review Comment:
This one is real. In the new pin, `RecordBatchTransformer` only converts
`List` and `LargeList` file columns to a `List` target, so a `FixedSizeList`
from a Parquet Arrow schema hint now errors with "cannot promote". It is
tracked upstream as apache/iceberg-rust#3471, and a fix is in progress. It only
affects files whose embedded Arrow schema uses `FixedSizeList`, which Spark and
Iceberg writers don't produce, so I'd like to merge as is and pick up the fix
with the next iceberg-rust bump. Does that work for you?
--
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]
