szehon-ho commented on code in PR #9335:
URL: https://github.com/apache/iceberg/pull/9335#discussion_r1444982918


##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -174,6 +175,8 @@ public static MetricsModes.MetricsMode metricsMode(
                           field.type(), 
file.upperBounds().get(field.fieldId()))));
 
   public static final String READABLE_METRICS = "readable_metrics";
+  public static final String REF_SNAPSHOT_ID = "reference_snapshot_id";

Review Comment:
   I feel like this constant is not too suitable in this file, which is for 
Metrics.  I think if we have to have a constant, should be in 
BaseMetadataTable.  Else we can just actually hard code it, it seems simpler.



##########
core/src/main/java/org/apache/iceberg/AllEntriesTable.java:
##########
@@ -64,9 +84,18 @@ protected TableScan newRefinedScan(Table table, Schema 
schema, TableScanContext
 
     @Override
     protected CloseableIterable<FileScanTask> doPlanFiles() {
-      CloseableIterable<ManifestFile> manifests =
-          reachableManifests(snapshot -> snapshot.allManifests(table().io()));
-      return BaseEntriesTable.planFiles(table(), manifests, tableSchema(), 
schema(), context());
+      return new ParallelIterable<>(

Review Comment:
   Looks like we don't close the ParallelIterable like in the original 
reachableManifests method. 
   
   Also looks like we are going to call planFiles many more times than the 
original.
   
   My first thought is to keep the logic but use a Pair<ManifestFile, Snapshot> 
where ManifestFile is used now.  How about adapting the correct 
reachableManifest code into a new more generic method like
   
   T traverse(Function<Snapshot, T> func)
   
   and have reachableManifests use that?



-- 
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-unsubscr...@iceberg.apache.org

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


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

Reply via email to