Re: [PR] Core: Optimize computing user-facing state in data task [iceberg]

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

findepi commented on code in PR #8346:
URL: https://github.com/apache/iceberg/pull/8346#discussion_r1350301245


##########
core/src/main/java/org/apache/iceberg/BaseFileScanTask.java:
##########
@@ -45,31 +50,67 @@ protected FileScanTask self() {
 
   @Override
   protected FileScanTask newSplitTask(FileScanTask parentTask, long offset, long length) {
-    return new SplitScanTask(offset, length, parentTask);
+    return new SplitScanTask(offset, length, deletesSizeBytes(), parentTask);
   }
 
   @Override
   public List<DeleteFile> deletes() {
-    return ImmutableList.copyOf(deletes);
+    if (deletesAsList == null) {
+      this.deletesAsList = Collections.unmodifiableList(Arrays.asList(deletes));
+    }
+
+    return deletesAsList;
+  }
+
+  @Override
+  public long sizeBytes() {
+    return length() + deletesSizeBytes();
+  }
+
+  @Override
+  public int filesCount() {
+    return 1 + deletes.length;

Review Comment:
   Why not initialize the deletes list in the constructor?
   Then no need to have these overrides. 
   
   That would mean BaseFileScanTask constructor needs to make a copy, but that's probably actually a good thing, since it would make the class more immutable.
   It will have to make the copy anyway, since `List deletes()` is the only way to get the deletes information from the class, so it will be called sooner or later.



##########
core/src/main/java/org/apache/iceberg/BaseFileScanTask.java:
##########
@@ -45,31 +47,66 @@ protected FileScanTask self() {
 
   @Override
   protected FileScanTask newSplitTask(FileScanTask parentTask, long offset, long length) {
-    return new SplitScanTask(offset, length, parentTask);
+    return new SplitScanTask(offset, length, parentTask, deletesSizeBytes());
   }
 
   @Override
   public List<DeleteFile> deletes() {
-    return ImmutableList.copyOf(deletes);

Review Comment:
   Just noticed that in the Iceberg version used in Trino, the deletes() does copy on every invocation. Thank you for fixing this!



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org