apache / apache/datafusion-comet
Iceberg serde: delete-file fields fall back to wrong defaults on reflection failure
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 190
Description
## Describe the bug
`CometIcebergNativeScan.extractDeleteFilesList` reads three delete-file fields by reflection and, if the reflective call throws, substitutes a default instead of failing. Serde runs after `CometScanRule` has already committed the plan to the native scan, so there is no fallback left at that point: the substituted default ships to iceberg-rust and the query returns a result that looks fine.
The three sites, all in `spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala`:
1. `:294-308` — `DeleteFile.content()` fails, the file is assumed to be `POSITION_DELETES`. An equality delete file applied as a position delete deletes rows by position, so the wrong rows are removed and the intended ones are not.
2. `:311-318` — `DeleteFile.specId()` fails, the spec id is recorded as `0`, binding the delete to whatever spec happens to be first rather than the one it was written under.
3. `:320-329` — `DeleteFile.equalityFieldIds()` fails, the equality ids are simply left off the protobuf. An equality delete with no equality keys cannot be applied, so deleted rows are returned to the user.
Finding 3 has a second, independent instance: `IcebergReflection.getEqualityFieldIds` (`spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:723-735`) swallows the same failure into an empty list, and that result also drives the task-schema decision at `CometIcebergNativeScan.scala:1061-1073`. So one reflection failure both strips the equality ids from the delete file and drops the equality-delete columns from the schema native projects, which is what `schemaWithRequiredFields` exists to prevent.
The surrounding code already takes the opposite position where it matters. The delete-file path is fatal (`:288`, "silently skipping it would leak deleted rows, so treat a missing path as fatal"), `extractDeleteFilesList` rethrows as a whole (`:337-343`), and `keyMetadataBytes` deliberately does not catch (`:249-254`). These three inner catches are the exceptions.
## Reachability
These are latent, not live. `content()`, `specId()` and `equalityFieldIds()` are all declared on the public `org.apache.iceberg.ContentFile` interface in every Iceberg version Comet builds against (verified with `javap` against iceberg-spark-runtime 1.5.2, 1.8.1 and 1.11.0), and the code resolves them against that interface rather than against Iceberg's package-private concrete file classes. So `getMethod` cannot throw today and the catches are unreachable.
The defect is the choice of default, which is what a future Iceberg version change would run into. A reader of this code cannot tell that `POSITION_DELETES` and `0` were picked as "unreachable, so it does not matter" rather than as "correct enough". That is worth closing before it becomes reachable.
## Expected behavior
Each of the three should fail the query rather than substitute a value, matching the delete-path handling immediately above them and `keyMetadataBytes`. The `getEqualityFieldIds` helper needs to distinguish "position delete, genuinely no equality ids" (empty list, correct) from "reflection threw" (fatal) rather than collapsing both to empty; see the companion issue on `IcebergReflection`'s inability to report why a lookup came back empty.
## Additional context
Found while auditing the serde path after review feedback on #5222, which noted that reflection failures during `CometScanRule` should fall back with a message but reflection failures at serde time must fail loudly rather than let a scan proceed with, for example, missing delete files.
Companion issues cover the schema/metadata helpers that return empty collections on failure, and the `IcebergReflection` API change that lets callers tell absence from failure.
Related: #5257 (schema/metadata helpers returning empty collections), #5258 (reflection helpers reporting absence vs failure).
Contributor guide
Research direction
Read the three reflection catches in spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala at the cited ranges, then inspect IcebergReflection.getEqualityFieldIds in spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala. Trace how these values reach extractDeleteFilesList and the task-schema decision. Done means reflection failures fail the query, while genuinely absent equality ids remain valid for position deletes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100