fix: make Iceberg delete-file reflection failures fatal - #5515
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Reviewed b474ade26a33e58d5c46f2d31a1b1a6d7f177d0d against eabb5d4773091b983d8fce713f0e34b1cf93f877. No P1/P2 findings.
Required-accessor failures now propagate instead of substituting delete metadata, while valid null positional equality IDs remain supported. The successful serialization and exception-cause behavior were checked with real Iceberg objects in isolated component controls across the four pinned dependency versions.
All six new tests also passed in the full Linux Spark 3.4/JDK 11 and Spark 4.0/JDK 21 CI runs. Those runs checked out the PR merge commit, whose tree matches this head; the local Scala 2.12/2.13 component checks are separate evidence, not full Spark/JNI runs.
CI is not all green: the macOS scans job failed with a native SIGBUS during thread cleanup before any recorded execution of the new serde suite. The supplied logs and crash artifacts did not establish a causal changed-line defect. That crash remains unattributed; this approval does not classify it as pre-existing.
Which issue does this PR close?
Closes #5256.
Rationale for this change
CometIcebergNativeScanserializes Iceberg delete-file metadata afterCometScanRulehas already committed the scan to native execution.Previously, reflection failures while reading three delete-file fields were silently replaced with defaults:
content()failure defaulted toPOSITION_DELETESspecId()failure defaulted to0equalityFieldIds()failure silently omitted the equality field IDsThese defaults are not safe. They can cause an equality delete to be interpreted as a position delete, bind a delete file to the wrong partition spec, or serialize an equality delete without its equality keys, potentially producing incorrect query results.
At serde time there is no Spark fallback left, so genuine reflection failures must fail the query rather than serialize guessed metadata.
What changes are included in this PR?
Remove the fallback-to-
POSITION_DELETESbehavior whencontent()reflection fails.Remove the fallback-to-
0behavior whenspecId()reflection fails.Stop swallowing failures from
equalityFieldIds().Preserve
nullreturned byequalityFieldIds()as a legitimate no-equality-keys result rather than treating it as a reflection failure.Extract single delete-file serialization into
serializeDeleteFileso the serde failure semantics can be tested directly.Add regression tests covering:
nullequality field IDs;content()invocation failures;specId()invocation failures;equalityFieldIds()invocation failures;Register the new serde regression suite in the Linux and macOS PR test matrices.
The behavior now matches the surrounding delete-file path and key-metadata handling: once native execution has been selected, required delete metadata is either serialized correctly or the query fails.
Compatibility
content(),specId(), andequalityFieldIds()are declared on Iceberg's publicContentFile/DeleteFileinterfaces across the Iceberg versions Comet currently supports.This therefore does not change normal behavior for supported Iceberg versions. It only removes unsafe defaults if reflective lookup or invocation fails.
How are these changes tested?
Added
CometIcebergDeleteFileSerdeSuitewith synthetic delete-file implementations that verify:equalityFieldIds()returnsnullstill serializes successfully with no equality IDs;content(),specId(), andequalityFieldIds()propagate instead of being replaced with defaults;The new suite is included in both Linux and macOS PR CI scan test buckets.