apache / apache/datafusion-comet
Iceberg serde: schema and metadata reflection helpers return empty collections on failure
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## Describe the bug
Three helpers reached from `CometIcebergNativeScan.serializePartitions` answer a reflection failure with an empty collection or a skipped field and a log line. Serde runs after `CometScanRule` has committed the plan to the native scan, so the empty answer is not a fallback: it is fed straight into the plan native executes.
### 1. `buildFieldIdMapping` returns an empty map
`spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:808-834` logs a warning and returns `Map.empty` if the schema walk fails, and drops individual columns from the map if a per-column lookup fails. At serde time that result feeds three places:
- `CometIcebergNativeScan.scala:1091-1104`, resolving output columns to field ids. A name missing from the mapping falls through to `metadata.globalFieldIdMapping` and then `MetadataFieldIds` before throwing, so a partial mapping can resolve a column to a different field id rather than failing.
- `:920-924` (`hasHistoricalColumns`), where an empty table-schema mapping makes the "does the scan reference dropped columns" test come out false and silently changes which schema the task is serialized with.
- `IcebergReflection.scala:543` (`schemaWithRequiredFields`), where an empty `existingIds` makes every equality-delete field id look absent from the task schema, sending the code down the schema-history resolution path unnecessarily and throwing there if history cannot supply them.
### 2. `pageIndexUnsupportedColumns` returns an empty set
`IcebergReflection.scala:847-870` logs a warning and returns `Set.empty`. Its own doc comment (`:838-846`) explains that residual predicates on decimal/uuid/fixed/binary columns either fail the native scan outright as an unsupported index type or panic it on a `String::from_utf8(..).unwrap()` over non-UTF-8 column-index bounds. An empty set means exactly those predicates get pushed. The failure is loud, but it surfaces as a native panic with no connection to the reflection failure that caused it.
### 3. `PartitionSpecParser.toJson` resolves to `None`
`CometIcebergNativeScan.scala:965-977` resolves the accessor into an `Option` and `:426-453` logs a per-task warning when it is absent, leaving `partition_spec_idx` unset while the partition *data* is still written unconditionally at `:529-544`. The comment at `:491-501` states native requires a task to carry both a spec and data or neither. `partition_spec_idx` is `optional` in `operator.proto:350` and `planner.rs:4031-4034` maps an unset index to `None`, so this is not silently aliased to pool entry 0 — it becomes an iceberg-rust error. As with (2) the outcome is a native failure whose message says nothing about the reflection problem, plus one warning per file scan task.
## Expected behavior
At serde time these should fail the query with a message naming the reflection failure, the way `extractDeleteFilesList:337-343` and `serializePartitionData:553-559` already do. The planning-path callers of the same helpers should keep the current quiet behavior, since `CometScanRule` still has the option to fall back. That split is what makes this non-trivial: `buildFieldIdMapping` and `pageIndexUnsupportedColumns` are called from both paths, so the fix needs either separate entry points or an error-returning variant rather than a blanket change.
## Additional context
Found while auditing the serde path after review feedback on #5222. Companion issues cover the delete-file fields that fall back to wrong defaults, and the `IcebergReflection` API change that lets callers tell "this Iceberg version lacks the accessor" from "the invoke threw".
Related: #5256 (delete-file defaults), #5258 (reflection helpers reporting absence vs failure).
Contributor guide
Research direction
Trace the serde callers in CometIcebergNativeScan.scala, especially serializePartitions and the partition-spec handling, then compare them with the planning-path callers in IcebergReflection.scala. Read the failure paths at IcebergReflection.scala:808-870, :543, and CometIcebergNativeScan.scala:426-453, :529-559, alongside operator.proto:350 and planner.rs:4031-4034. Done means serde reports reflection failures directly while planning retains its fallback behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, scala
- Domain
- backend, data-engineering
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100