[GitHub] [iceberg] szehon-ho commented on a diff in pull request #7613: Core: Minor metadata table code harmonization for readable_metrics

["szehon-ho (via GitHub)" <gi...@apache.org>]

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


##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -51,8 +51,8 @@ public Schema schema() {
     StructType partitionType = Partitioning.partitionType(table());
     Schema schema = ManifestEntry.getSchema(partitionType);
     if (partitionType.fields().size() < 1) {
-      // avoid returning an empty struct, which is not always supported. instead, drop the partition
-      // field (id 102)
+      // avoid returning an empty struct, which is not always supported.

Review Comment:
   This is not related, but fixing longstanding uneven line breaks introduced by the spotless refactor



##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -153,9 +150,7 @@ public CloseableIterable<StructLike> rows() {
 
     /**
      * Ensure that the underlying metrics used to populate readable metrics column are part of the
-     * file projection
-     *
-     * @return file projection with required columns to read readable metrics
+     * file projection.

Review Comment:
   I think the return does not convey additional information, so removed it in favor of the method comment for brevity



##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -133,16 +133,13 @@ public CloseableIterable<StructLike> rows() {
       Types.NestedField readableMetricsField = projection.findField(MetricsUtil.READABLE_METRICS);
 
       if (readableMetricsField == null) {
-        CloseableIterable<StructLike> entryAsStruct =
-            CloseableIterable.transform(
-                entries(fileProjection),
-                entry -> (GenericManifestEntry<? extends ContentFile<?>>) entry);
-
         StructProjection structProjection = structProjection(projection);
-        return CloseableIterable.transform(entryAsStruct, structProjection::wrap);
+
+        return CloseableIterable.transform(

Review Comment:
   I was trying to make both branches be more alike:
   
   1. calculate final struct projection
   2. calculate 'file' projection if needed for reading manifest
   3. Use these to transform the result



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