[GitHub] [iceberg] yyanyy commented on a change in pull request #2354: Core: add row key to format v2

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

yyanyy commented on a change in pull request #2354:
URL: https://github.com/apache/iceberg/pull/2354#discussion_r605980693



##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -482,6 +506,7 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
     // rebuild all of the partition specs and sort orders for the new current schema
     List<PartitionSpec> updatedSpecs = Lists.transform(specs, spec -> updateSpecSchema(newSchema, spec));
     List<SortOrder> updatedSortOrders = Lists.transform(sortOrders, order -> updateSortOrderSchema(newSchema, order));
+    RowKey updatedRowKey = updateRowKeySchema(newSchema, rowKey);

Review comment:
       should we fail if schema update drops column defined in the row key? probably want to add test for that too

##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -646,6 +659,55 @@ public void testUpdateSortOrder() {
         NullOrder.NULLS_FIRST, sortedByX.sortOrder().fields().get(0).nullOrder());
   }
 
+  @Test
+  public void testRowKey() {
+    Schema schema = new Schema(
+        Types.NestedField.required(10, "x", Types.StringType.get())
+    );
+
+    TableMetadata meta = TableMetadata.newTableMetadata(
+        schema, PartitionSpec.unpartitioned(), null, ImmutableMap.of());
+    RowKey rowKey = meta.rowKey();
+    Assert.assertTrue("Row key must be default", rowKey.isNotIdentified());
+  }
+
+  @Test
+  public void testUpdateRowKey() {
+    Schema schema = new Schema(
+        Types.NestedField.required(10, "x", Types.StringType.get()),
+        Types.NestedField.required(11, "y", Types.StringType.get())
+    );
+
+    RowKey id = RowKey.builderFor(schema).addField("x").build();
+
+    TableMetadata identifiedByX = TableMetadata.newTableMetadata(
+        schema, PartitionSpec.unpartitioned(), SortOrder.unsorted(), id, null, ImmutableMap.of());
+    RowKey actualKey = identifiedByX.rowKey();
+    Assert.assertEquals("Row key must have 1 field", 1, actualKey.identifierFields().size());
+    Assert.assertEquals("Row key must have the expected field",
+        org.apache.commons.compress.utils.Sets.newHashSet(1),

Review comment:
       nit: wrong import? Also may worth confirming "x" in schema is indeed assigned as id 1




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

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