Skip to content

fix: make Iceberg delete-file reflection failures fatal - #5515

Open
unikdahal wants to merge 1 commit into
apache:mainfrom
unikdahal:fix-iceberg-delete-file-reflection-defaults
Open

fix: make Iceberg delete-file reflection failures fatal#5515
unikdahal wants to merge 1 commit into
apache:mainfrom
unikdahal:fix-iceberg-delete-file-reflection-defaults

Conversation

@unikdahal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5256.

Rationale for this change

CometIcebergNativeScan serializes Iceberg delete-file metadata after CometScanRule has already committed the scan to native execution.

Previously, reflection failures while reading three delete-file fields were silently replaced with defaults:

  • content() failure defaulted to POSITION_DELETES
  • specId() failure defaulted to 0
  • equalityFieldIds() failure silently omitted the equality field IDs

These 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_DELETES behavior when content() reflection fails.

  • Remove the fallback-to-0 behavior when specId() reflection fails.

  • Stop swallowing failures from equalityFieldIds().

  • Preserve null returned by equalityFieldIds() as a legitimate no-equality-keys result rather than treating it as a reflection failure.

  • Extract single delete-file serialization into serializeDeleteFile so the serde failure semantics can be tested directly.

  • Add regression tests covering:

    • position deletes with null equality field IDs;
    • normal equality-delete serialization;
    • content() invocation failures;
    • specId() invocation failures;
    • equalityFieldIds() invocation failures;
    • missing required accessors.
  • 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(), and equalityFieldIds() are declared on Iceberg's public ContentFile/DeleteFile interfaces 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 CometIcebergDeleteFileSerdeSuite with synthetic delete-file implementations that verify:

  • a position delete whose equalityFieldIds() returns null still serializes successfully with no equality IDs;
  • equality delete field IDs are serialized unchanged;
  • failures from content(), specId(), and equalityFieldIds() propagate instead of being replaced with defaults;
  • a missing required accessor is treated as fatal.

The new suite is included in both Linux and macOS PR CI scan test buckets.

@coderfender
coderfender requested a review from mbutrovich August 28, 2026 05:00

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants