andygrove opened a new issue, #5256: URL: https://github.com/apache/datafusion-comet/issues/5256
## 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. -- 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]
