unikdahal opened a new pull request, #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515
## 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.
--
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]
