Copilot commented on code in PR #2435:
URL: https://github.com/apache/auron/pull/2435#discussion_r3687232176
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -559,6 +557,21 @@ object IcebergScanSupport extends Logging {
}
}
+ private def toNativeChangelogDataFileTask(
+ task: ChangelogScanTask): Option[NativeChangelogDataFileTask] = {
+ task match {
+ case added: AddedRowsScanTask
+ if added.operation() == ChangelogOperation.INSERT &&
+ deletesEmpty(added.deletes()) =>
+ Some(NativeChangelogDataFileTask(added.file(), added.start(),
added.length(), added))
+ case deleted: DeletedDataFileScanTask if
deletesEmpty(deleted.existingDeletes()) =>
+ Some(
+ NativeChangelogDataFileTask(deleted.file(), deleted.start(),
deleted.length(), deleted))
+ case _ =>
+ None
+ }
+ }
Review Comment:
`toNativeChangelogDataFileTask` accepts any `DeletedDataFileScanTask` as
long as `existingDeletes()` is empty, but the PR/issue requirements explicitly
limit native support to changelog tasks whose operation is `DELETE`. Without
checking `deleted.operation() == ChangelogOperation.DELETE`, non-DELETE
changelog tasks (e.g., future Iceberg task types that still implement
`DeletedDataFileScanTask`) could be incorrectly executed natively and emit the
wrong `_change_type` semantics.
##########
thirdparty/auron-iceberg/src/test/scala/org/apache/auron/iceberg/AuronIcebergIntegrationSuite.scala:
##########
@@ -700,39 +742,45 @@ class AuronIcebergIntegrationSuite
}
}
- test("iceberg changelog scan falls back when delete changes exist") {
- withTable("local.db.t_changelog_delete") {
- withTempView("t_changelog_delete_changes") {
+ test("iceberg native scan supports mixed insert and full-data-file delete
changelog scan") {
+ withTable("local.db.t_changelog_mixed_delete") {
+ withTempView("t_changelog_mixed_delete_changes") {
sql("""
- |create table local.db.t_changelog_delete (id int, v string)
+ |create table local.db.t_changelog_mixed_delete (id int, v
string, p int)
Review Comment:
The new changelog-native support boundary includes a critical fallback
condition: `DeletedDataFileScanTask` should only be handled when
`existingDeletes()` is empty. There isn’t a negative regression test that
produces a delete changelog task with non-empty `existingDeletes()` (or
otherwise introduces delete files) and asserts the plan falls back (no
`NativeIcebergTableScanExec`). Adding that test would help prevent
row-level/position/equality delete cases from accidentally becoming
native-supported.
--
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]
