apache / apache/datafusion-comet

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

Open
#5,256 1 comment 0 reactions 0 assignees View on GitHub
area:Iceberg area:scan bug correctness priority:medium
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.