Re: [PR] [AURON #2434] Support full-data-file Iceberg changelog deletes [auron]

["weiqingy (via GitHub)" <gi...@apache.org>]

weiqingy commented on code in PR #2435:
URL: https://github.com/apache/auron/pull/2435#discussion_r3694728354


##########
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:
   No change requested, just evidence on this one.
   
   The changelog task type that would actually be unsafe to run natively is `DeletedRowsScanTask`, the merge-on-read sibling that carries `addedDeletes()`. In Iceberg 1.10.1 it declares (javadoc stripped):
   
   ```java
   public interface DeletedRowsScanTask extends ChangelogScanTask, ContentScanTask<DataFile> {
     List<DeleteFile> addedDeletes();
   
     List<DeleteFile> existingDeletes();
   
     @Override
     default ChangelogOperation operation() {
       return ChangelogOperation.DELETE;
     }
   }
   ```
   
   It extends `ChangelogScanTask`, not `DeletedDataFileScanTask`, so the type match on this line is what keeps it off the native path. Its `operation()` is hard-coded `DELETE` as well, so an `operation()` check would not have excluded it either. Only the type match does.
   
   For the `_change_type` angle: the metadata case at `IcebergScanSupport.scala:620` reads the value straight off the task (`requiredChangelogTask(name).operation().name()`) rather than off the Scala type, so a subtype that overrode `operation()` would stamp its own value instead of a mismatched one.



-- 
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: commits-unsubscribe@auron.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org