Iceberg serde: delete-file fields fall back to wrong defaults on reflection failure

[andygrove]

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).

[mbutrovich]

Items 1-2 (content(), specId()): agree these are latent given the javap verification across 1.5.2/1.8.1/1.11.0. Worth noting CometScanRule has no analog call for either method either, so even if a future Iceberg release did break getMethod here, there's no planning-time path that would catch it first. If reachability ever changes, it lands directly at serde with the plan already committed. Item 3 (equalityFieldIds): the assumption that a planning-time reflection failure would already have triggered a fallback does not hold here. CometScanRule.scala:743-744 calls the same helper on the same object, but its blanket case _: Exception treats a genuine invoke failure the same as "no equality ids" and does not add a fallback reason (detail in the #5258 comment). Fixing #5258 is a prerequisite for this one to actually explode loudly at serde instead of silently defaulting at both call sites.