You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "unikdahal (via GitHub)" <gi...@apache.org> on 2026/08/27 22:52:55 UTC
[PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5608492198
We should also make sure we throw on the `run-iceberg-tests` label on PRs that touch core Iceberg logic. I just added it and will take a look at this tomorrow.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5705869069
@mbutrovich Thanks, addressed the remaining feedback. Updated the PR description to clarify the position-delete vs equality-delete `equalityFieldIds()` behavior, added the enum-value regression coverage, and filed #5992 for the residual-expression reflection case.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5608818257
Rebased on latest `main`. Could someone trigger CI while @mbutrovich gets a chance to review the latest changes?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#discussion_r3907561892
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +293,63 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString match {
+ case IcebergReflection.ContentTypes.POSITION_DELETES =>
+ IcebergReflection.ContentTypes.POSITION_DELETES
+ case IcebergReflection.ContentTypes.EQUALITY_DELETES =>
+ IcebergReflection.ContentTypes.EQUALITY_DELETES
+ case other => other
+ }
Review Comment:
Yep, agreed. Removed the match and now just serialize `content().toString` directly. The native planner remains responsible for validating the content type.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#discussion_r3982090906
##########
spark/src/test/scala/org/apache/comet/serde/operator/CometIcebergDeleteFileSerdeSuite.scala:
##########
@@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde.operator
+
+import java.lang.reflect.InvocationTargetException
+
+import org.scalatest.funsuite.AnyFunSuite
+
+import org.apache.iceberg.DeleteFile
+
+import org.apache.comet.iceberg.IcebergReflection
+
+/**
+ * Locks in the fail-loud behavior of [[CometIcebergNativeScan.serializeDeleteFile]] required by
+ * apache/datafusion-comet#5256: on supported Iceberg versions `content()`, `specId()`, and
+ * `equalityFieldIds()` are always declared, so a reflective lookup or invocation failure must
+ * propagate rather than fall back to a guessed value. A null `equalityFieldIds()` (a
+ * position-delete file) stays a legitimate "no equality keys" result.
+ */
+class CometIcebergDeleteFileSerdeSuite extends AnyFunSuite {
+
+ private def keyMetadataMethod(clazz: Class[_]) = clazz.getMethod("keyMetadata")
+
+ private def serialize(file: AnyRef) =
+ CometIcebergNativeScan.serializeDeleteFile(
+ file,
+ file.getClass,
+ file.getClass,
+ keyMetadataMethod(file.getClass))
+
+ test("required Iceberg delete-file accessors are present") {
Review Comment:
This is the test that closes the version-bump gap, good addition. Could it assert the values too, not only that the methods exist?
```scala
assert(FileContent.POSITION_DELETES.toString == IcebergReflection.ContentTypes.POSITION_DELETES)
assert(FileContent.EQUALITY_DELETES.toString == IcebergReflection.ContentTypes.EQUALITY_DELETES)
```
`serializeDeleteFile` now sends `content().toString` straight into the proto and `planner.rs:4364` matches those two literals, so the string values are as load-bearing as the accessors are. The stubs all declare `def content(): String`, so nothing in the suite touches the real enum. `org.apache.iceberg.FileContent` is on the test classpath already, same jar as the `DeleteFile` import on line 26.
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +306,56 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString
Review Comment:
Dropping the match is right, the real validation is already on the native side at `planner.rs:4364`. One consequence worth covering though. `contentType` is now a raw pass-through of the enum's `name()`, and `planner.rs` matches those strings exactly.
I checked `iceberg-spark-runtime-4.1_2.13-1.11.0` with javap. `ContentFile.content()` returns `FileContent`, and `FileContent` declares `POSITION_DELETES` and `EQUALITY_DELETES` without overriding `toString`, so the names line up today. Nothing in the suite asserts that. A constant rename in a future Iceberg would pass the new accessor-existence test and then fail every query that has delete files.
Separately, after this hunk `IcebergReflection.ContentTypes.POSITION_DELETES` has no callers left. A grep over `spark/`, `common/` and `native/` only finds the three occurrences this hunk deletes, and line 345 is the last use of `EQUALITY_DELETES`. Pinning the names in a test (see my comment on the suite) would give the constant a use again. Otherwise it should probably be removed.
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +306,56 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString
+ deleteBuilder.setContentType(contentType)
+
+ val specIdMethod = IcebergReflection.getMethod(deleteFileClass, "specId")
+ deleteBuilder.setPartitionSpecId(specIdMethod.invoke(deleteFile).asInstanceOf[Int])
+
+ val equalityFieldIds = requiredEqualityFieldIds(deleteFileClass, deleteFile)
+ val isEqualityDelete =
+ contentType == IcebergReflection.ContentTypes.EQUALITY_DELETES
+ if (isEqualityDelete && equalityFieldIds.isEmpty) {
Review Comment:
This is a new fatal case rather than one of the three fallbacks the PR removes. Before, an equality delete with empty ids serialized with no equality ids. Now it fails the query. I think that is correct, an equality delete with no keys cannot be applied, so failing beats guessing.
It is not in the description though. The `equalityFieldIds()` bullet currently says `null` is preserved as a legitimate no-equality-keys result. That holds for position deletes only. For an equality delete, `null` and empty are both fatal after this change. Could you add this to the changes list and adjust that bullet? The description becomes the squash commit message on a `Closes #5256` fix, so it is worth having it match what landed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#discussion_r3899609342
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +293,63 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString match {
+ case IcebergReflection.ContentTypes.POSITION_DELETES =>
+ IcebergReflection.ContentTypes.POSITION_DELETES
+ case IcebergReflection.ContentTypes.EQUALITY_DELETES =>
+ IcebergReflection.ContentTypes.EQUALITY_DELETES
+ case other => other
+ }
+ deleteBuilder.setContentType(contentType)
+
+ val specIdMethod = IcebergReflection.getMethod(deleteFileClass, "specId")
+ deleteBuilder.setPartitionSpecId(specIdMethod.invoke(deleteFile).asInstanceOf[Int])
+
+ val equalityIdsMethod = IcebergReflection.getMethod(deleteFileClass, "equalityFieldIds")
+ val equalityIds = equalityIdsMethod.invoke(deleteFile).asInstanceOf[java.util.List[Integer]]
+ // Iceberg's BaseFile stores equality field IDs in a nullable backing array, so
+ // equalityFieldIds() returns null for files without equality keys. A null return is a
+ // normal accessor result, unlike a reflective lookup or invocation failure, and does not
+ // make serialization fail.
+ if (equalityIds != null) {
+ equalityIds.forEach(id => deleteBuilder.addEqualityIds(id))
+ }
Review Comment:
Nice catch on the `null` return here. I checked `BaseFile.equalityFieldIds()` and it goes through `ArrayUtil.toUnmodifiableIntList`, which returns `null` for a null backing array, so the old `catch` really was swallowing an NPE on every position-delete file. Without this guard the PR would break every position delete.
One thing though. `IcebergReflection.getEqualityFieldIds` does almost exactly this, including the null-to-empty mapping, but keeps the blanket catch. `CometIcebergNativeScan` still calls it a bit further up to decide whether to union equality-delete field IDs into the task schema via `schemaWithRequiredFields`. That is the second instance #5256 calls out under finding 3:
> Finding 3 has a second, independent instance: `IcebergReflection.getEqualityFieldIds` swallows the same failure into an empty list, and that result also drives the task-schema decision.
and its Expected behavior section asks for the helper to distinguish a genuine "no equality ids" from a reflective failure.
Would it work to drop the catch from `getEqualityFieldIds`, keep the null-to-empty mapping there, and have `serializeDeleteFile` call it? That fixes both call sites and avoids carrying two copies of this reflection with different failure semantics.
There is a wrinkle with the third caller. `CometScanRule` calls the same helper during planning, where falling back to Spark is still an option, so a blanket change would turn a fallback into a failure there. That is the same split-entry-point problem #5257 describes for `buildFieldIdMapping`, so it may be cleaner as a separate change.
If you would rather keep this PR tight, could we switch the description to "Part of #5256" and file a follow-up for the helper? I checked and it is not covered by #5257, which enumerates `buildFieldIdMapping`, `pageIndexUnsupportedColumns` and `PartitionSpecParser.toJson` but not this one, and #5258 is already closed. So as written the issue would get closed with half of finding 3 still open.
##########
spark/src/test/scala/org/apache/comet/serde/operator/CometIcebergDeleteFileSerdeSuite.scala:
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde.operator
+
+import java.lang.reflect.InvocationTargetException
+
+import org.scalatest.funsuite.AnyFunSuite
+
+/**
+ * Locks in the fail-loud behavior of [[CometIcebergNativeScan.serializeDeleteFile]] required by
+ * apache/datafusion-comet#5256: on supported Iceberg versions `content()`, `specId()`, and
+ * `equalityFieldIds()` are always declared, so a reflective invocation failure must propagate
+ * rather than fall back to a guessed value. A null `equalityFieldIds()` (a position-delete file)
+ * stays a legitimate "no equality keys" result.
+ */
+class CometIcebergDeleteFileSerdeSuite extends AnyFunSuite {
+
+ private def keyMetadataMethod(clazz: Class[_]) = clazz.getMethod("keyMetadata")
+
+ private def serialize(file: AnyRef) =
+ CometIcebergNativeScan.serializeDeleteFile(
+ file,
+ file.getClass,
+ file.getClass,
+ keyMetadataMethod(file.getClass))
+
+ test("position-delete file: null equalityFieldIds() serializes with no equality ids") {
+ val proto = serialize(new PositionDeleteFile)
+ assert(proto.getContentType == "POSITION_DELETES")
+ assert(proto.getPartitionSpecId == 7)
+ assert(proto.getEqualityIdsCount == 0)
+ assert(proto.getFilePath == "s3://bucket/pos-delete.parquet")
+ }
+
+ test("equality-delete file: declared equalityFieldIds() are serialized") {
+ val proto = serialize(new EqualityDeleteFile)
+ assert(proto.getContentType == "EQUALITY_DELETES")
+ assert(proto.getEqualityIdsCount == 2)
+ assert(proto.getEqualityIds(0) == 3)
+ assert(proto.getEqualityIds(1) == 5)
+ }
+
+ test("content() invocation failure propagates instead of defaulting to POSITION_DELETES") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingContentDeleteFile))
+ assert(ex.getCause.getMessage == "content boom")
+ }
+
+ test("specId() invocation failure propagates instead of defaulting to 0") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingSpecIdDeleteFile))
+ assert(ex.getCause.getMessage == "spec boom")
+ }
+
+ test("equalityFieldIds() invocation failure propagates instead of dropping equality ids") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingEqualityIdsDeleteFile))
+ assert(ex.getCause.getMessage == "ids boom")
+ }
+
+ test("missing content() accessor is fatal, not a default") {
+ // getMethod throws NoSuchMethodException directly (not wrapped) when the accessor is absent.
+ assertThrows[NoSuchMethodException](serialize(new NoContentAccessorDeleteFile))
+ }
Review Comment:
The synthetic stubs are a clean way to pin the control flow, thanks for that.
The argument for removing the fallbacks is that `content()`, `specId()` and `equalityFieldIds()` are always declared on the real Iceberg interfaces, and I do not think anything in the suite checks that. I verified it holds with `javap` across all four pinned versions, and they are all on `ContentFile`. But the consequence of that ceasing to be true changed with this PR. It used to be quietly wrong rows, now it is a hard query failure for anyone with delete files.
That is the regression that matters most now, and it is the one a test would let CI catch at the version bump rather than in the field. The pieces are already here, `iceberg-spark-runtime` is a test dependency in every Spark profile and `IcebergReflectionSuite` already resolves methods against real Iceberg classes. Would you add something like `assert(IcebergReflection.findMethod(classOf[DeleteFile], "content").isDefined)` for each of the three?
Also, the missing-path branch in `serializeDeleteFile` is the one fail-loud path without a test, and it is the one carrying the longest comment about why it has to be fatal. A stub declaring neither `location()` nor `path()` would round the suite out.
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +293,63 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString match {
+ case IcebergReflection.ContentTypes.POSITION_DELETES =>
+ IcebergReflection.ContentTypes.POSITION_DELETES
+ case IcebergReflection.ContentTypes.EQUALITY_DELETES =>
+ IcebergReflection.ContentTypes.EQUALITY_DELETES
+ case other => other
+ }
Review Comment:
All three branches of this match return their argument unchanged, so the whole thing is equivalent to `contentMethod.invoke(deleteFile).toString`.
The real validation is already on the native side. `planner.rs:3910` rejects any content type that is not one of these two with `Invalid delete content type '{}'`. Since you are rewriting this block anyway, could we drop the match? As it stands it reads like the Scala side is filtering when it is not.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5486621300
Let's also give @mbutrovich an opportunity to review this PR
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#discussion_r3907558376
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -346,6 +293,63 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
}
}
+ /**
+ * Serializes a single Iceberg DeleteFile to protobuf.
+ *
+ * `content()`, `specId()`, and `equalityFieldIds()` are declared on the public `ContentFile` /
+ * `DeleteFile` interfaces across all supported Iceberg versions, so a `getMethod` miss or an
+ * `invoke` failure on any of them means something is genuinely wrong. None of the three may
+ * fall back to a default: the scan is already committed to native execution, and a guessed
+ * content type, partition spec, or dropped equality keys all silently return wrong rows.
+ * Failures propagate to `extractDeleteFilesList`'s outer catch.
+ */
+ private[operator] def serializeDeleteFile(
+ deleteFile: Any,
+ contentFileClass: Class[_],
+ deleteFileClass: Class[_],
+ keyMetadataMethod: Method): OperatorOuterClass.IcebergDeleteFile = {
+ // The path is the one essential field. A delete file we cannot locate cannot be applied,
+ // and silently skipping it would leak deleted rows, so treat a missing path as fatal.
+ val deletePath = IcebergReflection
+ .extractFileLocation(contentFileClass, deleteFile)
+ .getOrElse(
+ throw new RuntimeException(
+ "Neither location() nor path() is declared on this Iceberg version's " +
+ "ContentFile -- cannot extract delete file path from FileScanTask"))
+
+ val deleteBuilder = OperatorOuterClass.IcebergDeleteFile.newBuilder()
+ deleteBuilder.setFilePath(deletePath)
+
+ val contentMethod = IcebergReflection.getMethod(deleteFileClass, "content")
+ val contentType = contentMethod.invoke(deleteFile).toString match {
+ case IcebergReflection.ContentTypes.POSITION_DELETES =>
+ IcebergReflection.ContentTypes.POSITION_DELETES
+ case IcebergReflection.ContentTypes.EQUALITY_DELETES =>
+ IcebergReflection.ContentTypes.EQUALITY_DELETES
+ case other => other
+ }
+ deleteBuilder.setContentType(contentType)
+
+ val specIdMethod = IcebergReflection.getMethod(deleteFileClass, "specId")
+ deleteBuilder.setPartitionSpecId(specIdMethod.invoke(deleteFile).asInstanceOf[Int])
+
+ val equalityIdsMethod = IcebergReflection.getMethod(deleteFileClass, "equalityFieldIds")
+ val equalityIds = equalityIdsMethod.invoke(deleteFile).asInstanceOf[java.util.List[Integer]]
+ // Iceberg's BaseFile stores equality field IDs in a nullable backing array, so
+ // equalityFieldIds() returns null for files without equality keys. A null return is a
+ // normal accessor result, unlike a reflective lookup or invocation failure, and does not
+ // make serialization fail.
+ if (equalityIds != null) {
+ equalityIds.forEach(id => deleteBuilder.addEqualityIds(id))
+ }
Review Comment:
Good point. I kept the planning helper unchanged since `CometScanRule` still needs to be able to fall back to Spark.
I added a strict serde-side helper instead and use it in both `serializeDeleteFile` and the task-schema union path. So lookup/invocation failures are fatal once native execution is committed, while a null return is still treated as empty for position deletes.
This should cover the second `#5256` path without changing the planning-time fallback behavior.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#discussion_r3907567401
##########
spark/src/test/scala/org/apache/comet/serde/operator/CometIcebergDeleteFileSerdeSuite.scala:
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde.operator
+
+import java.lang.reflect.InvocationTargetException
+
+import org.scalatest.funsuite.AnyFunSuite
+
+/**
+ * Locks in the fail-loud behavior of [[CometIcebergNativeScan.serializeDeleteFile]] required by
+ * apache/datafusion-comet#5256: on supported Iceberg versions `content()`, `specId()`, and
+ * `equalityFieldIds()` are always declared, so a reflective invocation failure must propagate
+ * rather than fall back to a guessed value. A null `equalityFieldIds()` (a position-delete file)
+ * stays a legitimate "no equality keys" result.
+ */
+class CometIcebergDeleteFileSerdeSuite extends AnyFunSuite {
+
+ private def keyMetadataMethod(clazz: Class[_]) = clazz.getMethod("keyMetadata")
+
+ private def serialize(file: AnyRef) =
+ CometIcebergNativeScan.serializeDeleteFile(
+ file,
+ file.getClass,
+ file.getClass,
+ keyMetadataMethod(file.getClass))
+
+ test("position-delete file: null equalityFieldIds() serializes with no equality ids") {
+ val proto = serialize(new PositionDeleteFile)
+ assert(proto.getContentType == "POSITION_DELETES")
+ assert(proto.getPartitionSpecId == 7)
+ assert(proto.getEqualityIdsCount == 0)
+ assert(proto.getFilePath == "s3://bucket/pos-delete.parquet")
+ }
+
+ test("equality-delete file: declared equalityFieldIds() are serialized") {
+ val proto = serialize(new EqualityDeleteFile)
+ assert(proto.getContentType == "EQUALITY_DELETES")
+ assert(proto.getEqualityIdsCount == 2)
+ assert(proto.getEqualityIds(0) == 3)
+ assert(proto.getEqualityIds(1) == 5)
+ }
+
+ test("content() invocation failure propagates instead of defaulting to POSITION_DELETES") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingContentDeleteFile))
+ assert(ex.getCause.getMessage == "content boom")
+ }
+
+ test("specId() invocation failure propagates instead of defaulting to 0") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingSpecIdDeleteFile))
+ assert(ex.getCause.getMessage == "spec boom")
+ }
+
+ test("equalityFieldIds() invocation failure propagates instead of dropping equality ids") {
+ val ex = intercept[InvocationTargetException](serialize(new ThrowingEqualityIdsDeleteFile))
+ assert(ex.getCause.getMessage == "ids boom")
+ }
+
+ test("missing content() accessor is fatal, not a default") {
+ // getMethod throws NoSuchMethodException directly (not wrapped) when the accessor is absent.
+ assertThrows[NoSuchMethodException](serialize(new NoContentAccessorDeleteFile))
+ }
Review Comment:
Added both. The suite now checks `content`, `specId`, and `equalityFieldIds` against the real `DeleteFile` API, and there is a regression test verifying that a delete file with neither `location()` nor `path()` fails loudly.
I also added coverage for a missing `equalityFieldIds()` accessor.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5619946281
I stopped CI since it looks like formatting was failing.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5957096429
@sunchao I’ve resolved the conflicts. Could you please trigger CI when you get a chance, and merge it if everything looks good? Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5920284998
@unikdahal could you fix the conflicts? after that I can merge the PR. thanks
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: make Iceberg delete-file reflection failures fatal [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5515:
URL: https://github.com/apache/datafusion-comet/pull/5515#issuecomment-5804281872
@andygrove @mbutrovich Could you please retrigger CI? Looks like an unrelated Gradle download failure.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org