[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #8263: Core: Use lazy iterable in DeleteFileIndex

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

aokolnychyi commented on code in PR #8263:
URL: https://github.com/apache/iceberg/pull/8263#discussion_r1288723879


##########
core/src/main/java/org/apache/iceberg/metrics/ScanMetrics.java:
##########
@@ -127,6 +129,16 @@ public Counter positionalDeleteFiles() {
     return metricsContext().counter(POSITIONAL_DELETE_FILES);
   }
 
+  public void indexedDeleteFile(DeleteFile deleteFile) {

Review Comment:
   I wasn't sure myself. It felt to me a few similar methods like this would be appropriate to hide the complexity of populating the same values from multiple places. If we go with this one, I'd expect more methods. For instance, we could have one that accepts a matching data file and updates both the count as well as the total file size. A similar thing can be done for the array of delete files, etc.
   
   I see a few potential benefits:
   - Less metrics related code (1 line vs 14 lines in the delete index), moving the complexity to `ScanMetrics`.
   - Methods can be shared (e.g. distributed planning updates exactly the same metrics as `ManifestGroup`).
   - We can add more metrics implicitly without requiring changes outside of `ScanMetrics`.
   
   That said, I don't feel strongly. How did you envision this API, @nastra?



-- 
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