[GitHub] [iceberg] kbendick commented on a diff in pull request #4847: Core: Add reference_snapshot_id column and filter to AllManifest table

[GitBox <gi...@apache.org>]

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