kbendick commented on code in PR #4847:
URL: https://github.com/apache/iceberg/pull/4847#discussion_r907921417


##########
core/src/main/java/org/apache/iceberg/AllManifestsTable.java:
##########
@@ -43,6 +52,7 @@
  * This table may return duplicate rows.
  */
 public class AllManifestsTable extends BaseMetadataTable {
+  private static final int REF_SNAPSHOT_ID = 18;

Review Comment:
   Side comment / non-blocking: I'm a _big_ fan of using named constants for 
schema element IDs.
   
   Should we update to use named position constants throughout, particularly 
for schemas like the metadata tables that are evolving (note that `content` is 
listed first but has ID 14). But is the cherry-picking that would be required 
by a large diff like this considered an unnecessary change?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to