You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/06 17:24:28 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4716: [WIP] [CORE] - Add remaining parsers for MetadataUpdate implementations
kbendick opened a new pull request, #4716:
URL: https://github.com/apache/iceberg/pull/4716
Adds the remaining parsers for metadata update subclasses.
Opening this as a draft so I can compare and because some field renaming in a tangentially related PR: https://github.com/apache/iceberg/pull/4693
I'll probably move the field renaming over here if they don't get merged in over there before.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876269811
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +79,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref-name";
Review Comment:
We updated this field from `ref` to `ref-name` in the spec.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876263324
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,120 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate update = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Remove snapshots should serialize to the correct JSON value", expected, actual);
+ }
+
+
+ // TODO - SetSnapshotRef tests.
+
+ @Test
+ public void testSetPropertiesFromJson() {
+ String action = MetadataUpdateParser.SET_PROPERTIES;
+ Map<String, String> props = Maps.newHashMap();
+ props.put("prop1", "val1");
+ props.put("prop2", null);
+ String propsMap = "{\"prop1\":\"val1\",\"prop2\":null}";
Review Comment:
Changed to ImmutableMap and added a test for throwing when reading `null`.
Not allowing `null` as a value _before converting to json_ seems like something that the `MetadataUpdate.SetProperties` class should be responsible for. So I only tested in `fromJson`.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877585164
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
+
+ private static void assertEqualsRemoveSnapshots(
+ MetadataUpdate.RemoveSnapshot expected, MetadataUpdate.RemoveSnapshot actual) {
+ Assert.assertEquals("Snapshots to remove should be the same", expected.snapshotId(), actual.snapshotId());
+ }
+
+ // See TODOs in MetadataUpdate.SetSnapshotRef.
+ private static void assertEqualsSetSnapshotRef(
+ MetadataUpdate.SetSnapshotRef expected, MetadataUpdate.SetSnapshotRef actual) {
+ Assert.fail("MetadataUpdate equality checking for SetSnapshotRef is not implemented yet");
+ }
Review Comment:
I have added all of those tests. I basically copied the tests from `TestSnapshotRefParser` and updated them for this, with versions with versions that read all cases.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876246180
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -233,25 +233,25 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
}
class SetSnapshotRef implements MetadataUpdate {
- private final String name;
+ private final String refName;
Review Comment:
Given we updated the spec to use `ref-name`, I also changed the field name to `refName`.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r879791312
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
Review Comment:
Yeah I moved forward with just 1 allowed. Would you like me to change the class name to be singular for now? Currently the singular constructor class is `RemovePartitions`.
We can either accept one value and then collect it into a set and expose two constructors or make two classes.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875285210
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
Review Comment:
Should I implement `AddSnapshot` in another PR instead? We still need to plumb through `FileIO` to `BaseSnapshot` when parsing from json so I think probably so.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875312043
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
Review Comment:
I think this should check that the snapshot ID, manifest list location, and summary map are equal.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875304580
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
Review Comment:
When could this be null? I think this is correct (so this comment is minor) but I'd normally expect to use `getLogSet` that handles the null case internally. This is fine because there's a good error message.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875304026
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
Review Comment:
Is `%o` correct? I would expect `%s` or maybe `%d`.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875239674
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
Review Comment:
TODO / Follow up - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot methods can be reused. Also, this possibly needs to handle the difference between V1 vs V2 for assertions. Or casting to BaseSnapshot, BaseSnapshot::equals can be used.
Happy to remove this in this PR and handle `EqualsAddSnapshot` in another PR.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876246180
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -233,25 +233,25 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
}
class SetSnapshotRef implements MetadataUpdate {
- private final String name;
+ private final String refName;
Review Comment:
Given we updated the spec to use `ref-name`, I also changed the field name and the method name.
I can do this in a separate PR (as well as update the spec in that same PR) or I can leave the class's code as is for now. WDYT? Changing it here does admittedly make the PR's scope somewhat broad for what it touches.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876267361
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,138 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long snapshotId = 2L;
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":[2]}", action);
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long snapshotId = 2L;
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":[2]}", action);
+ MetadataUpdate update = new MetadataUpdate.RemoveSnapshot(snapshotId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Remove snapshots should serialize to the correct JSON value", expected, actual);
+ }
+
+
+ // TODO - SetSnapshotRef tests.
+
+ @Test
+ public void testSetPropertiesFromJson() {
+ String action = MetadataUpdateParser.SET_PROPERTIES;
+ Map<String, String> props = ImmutableMap.of(
+ "prop1", "val1",
+ "prop2", "val2"
+ );
+ String propsMap = "{\"prop1\":\"val1\",\"prop2\":\"val2\"}";
+ String json = String.format("{\"action\":\"%s\",\"updated\":%s}", action, propsMap);
+ MetadataUpdate expected = new MetadataUpdate.SetProperties(props);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ @Test
+ public void testSetPropertiesFromJson_FailsDeserializingNullValue() {
+ String action = MetadataUpdateParser.SET_PROPERTIES;
+ Map<String, String> props = Maps.newHashMap();
+ props.put("prop1", "val1");
+ props.put("prop2", null);
+ String propsMap = "{\"prop1\":\"val1\",\"prop2\":null}";
+ String json = String.format("{\"action\":\"%s\",\"updated\":%s}", action, propsMap);
+ AssertHelpers.assertThrows(
+ "Parsing updates from SetProperties with a property set to null should throw",
+ IllegalArgumentException.class,
+ "Cannot parse prop2 to a string value: null",
Review Comment:
This is the error message thrown, although combined with the full stack trace I think it should be clear as we'll have `MetadataUpdateParser.fromJson` in it.
We might want to move some of the validation logic into `MetadataUpdate.SetProperties` though, particularly when the Java object is being created (so when the client would be sending the payload using `toJson` instead).
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876207127
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
Review Comment:
You're correct. This should never be null (as evidenced by the null precondition check below).
I added the `getLongSetOrNull` function essentially because it mirrored the `getIntegerSetOrNull` function and still needed a preconditions check here for the size (given we don't currently support multiple values).
I can change to `getLongSet` given the utility method is added in this PR, but the Precondition check will still be needed.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875305124
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(
+ refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
Review Comment:
Values can be `null`? Is that allowed in a normal table?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875308270
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
Review Comment:
Move forward with just 1 allowed, temporarily. I think in a follow up PR, we should support multiple snapshots in `RemoveSnapshot` (renamed to `RemoveSnapshots`).
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876200040
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
The line for `MIN_SNAPSHOTS_TO_KEEP` can't fit on one line. Everything else seems like it can.
Updating.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876227805
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
+1. Do we also want to change the corresponding update requirement field? We had the same concern there.
https://github.com/apache/iceberg/blob/3586e140dc3fbe0b0414464497be3ccd4ca0927d/core/src/main/java/org/apache/iceberg/rest/requests/UpdateRequirementParser.java#L55
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875231131
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -234,6 +234,7 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
class SetSnapshotRef implements MetadataUpdate {
private final String name;
+ // TODO - this can be primitive.
private final Long snapshotId;
Review Comment:
TODO / possible follow up - This can be made primitive
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875286129
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
Review Comment:
Currently, the spec allows for `RemoveSnapshots` to have multiple snapshots. But the code only allows for one.
Should we implement in another PR, or move forward with just 1 allowed temporarily?
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877518363
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -250,6 +251,8 @@ public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type, Intege
this.maxRefAgeMs = maxRefAgeMs;
}
+ // TODO - Consider renaming to `refName`, as the JSON is called `ref`. This also matches the equivalent
+ // UpdateRequirement.
public String name() {
Review Comment:
I did update this to `refName`, as we updated the spec to refer to this field as `ref-name` instead of simply `name`.
I can revert the changes to the code though.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878529908
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
Review Comment:
I added timestamp, schema id and parent id as well as the ones you stated. That should be more than sufficient to verify that the existing parser is working (which we reuse).
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878343297
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -233,25 +233,25 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
}
class SetSnapshotRef implements MetadataUpdate {
- private final String name;
+ private final String refName;
private final Long snapshotId;
private final SnapshotRefType type;
private Integer minSnapshotsToKeep;
private Long maxSnapshotAgeMs;
private Long maxRefAgeMs;
- public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type, Integer minSnapshotsToKeep,
+ public SetSnapshotRef(String refName, Long snapshotId, SnapshotRefType type, Integer minSnapshotsToKeep,
Long maxSnapshotAgeMs, Long maxRefAgeMs) {
- this.name = name;
+ this.refName = refName;
this.snapshotId = snapshotId;
this.type = type;
this.minSnapshotsToKeep = minSnapshotsToKeep;
this.maxSnapshotAgeMs = maxSnapshotAgeMs;
this.maxRefAgeMs = maxRefAgeMs;
}
- public String name() {
- return name;
+ public String refName() {
Review Comment:
Probably better not to rename this, since it requires touching two extra files.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875309691
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,120 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate update = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Remove snapshots should serialize to the correct JSON value", expected, actual);
+ }
+
+
+ // TODO - SetSnapshotRef tests.
+
+ @Test
+ public void testSetPropertiesFromJson() {
+ String action = MetadataUpdateParser.SET_PROPERTIES;
+ Map<String, String> props = Maps.newHashMap();
+ props.put("prop1", "val1");
+ props.put("prop2", null);
+ String propsMap = "{\"prop1\":\"val1\",\"prop2\":null}";
Review Comment:
Probably don't need a null test.
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,120 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate update = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Remove snapshots should serialize to the correct JSON value", expected, actual);
+ }
+
+
+ // TODO - SetSnapshotRef tests.
+
+ @Test
+ public void testSetPropertiesFromJson() {
+ String action = MetadataUpdateParser.SET_PROPERTIES;
+ Map<String, String> props = Maps.newHashMap();
+ props.put("prop1", "val1");
+ props.put("prop2", null);
+ String propsMap = "{\"prop1\":\"val1\",\"prop2\":null}";
Review Comment:
Probably don't need a null test, except to disallow it.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875306185
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(
+ refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
+
+ private static MetadataUpdate readRemoveProperties(JsonNode node) {
+ Preconditions.checkArgument(node.hasNonNull(REMOVED), "Missing field: removed");
+ JsonNode removedNode = node.get(REMOVED);
+ String[] removed = JsonUtil.getStringArray(removedNode);
Review Comment:
What about using `getStringSet`? There's a `getLongSetOrNull` method.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875300775
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
I think they should be omitted.
Also, do these need to be wrapped? It looks like they would fit on one line.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875233285
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
Question: Should we explicitly set the `null` values or not?
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876207127
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
Review Comment:
You're correct. This should never be null (as evidence by the null check below).
I added the `getLongSetOrNull` function essentially because it mirrored the `getIntegerSetOrNull` function and still needed a preconditions check here for the size (given we don't currently support multiple values).
I can update to `getLongSet` given it's added in this PR, but the Precondition check will still be needed.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878529009
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -234,6 +234,7 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
class SetSnapshotRef implements MetadataUpdate {
private final String name;
+ // TODO - this can be primitive.
private final Long snapshotId;
Review Comment:
This TODO is no longer written in the code, but doesn't show as outdated because the comment is on the line below.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877539121
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +502,52 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
Review Comment:
reverted.
--
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
[GitHub] [iceberg] kbendick commented on pull request #4716: [WIP] [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#issuecomment-1120585935
Given this is a much larger change set than I'd expected, and one or two of the classes need to be aligned with the spec, I'm going to use this PR as a template and open a series of PRs.
I'll add tasks to that to make it easier to track but the general goal is:
1) Full shell of all needed functions, so individual changes are easy to make.
2) Add implementations and tests for the simplest classes
3) Add implementations and tests for any classes that are simple except for needing to update `JsonUtil`
4) Open a PR for classes that require a closer look, like `RemoveSnapshots` which allows for a set of snapshotIds to remove per the spec, but the implementation only accepts one value.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876217105
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
Review Comment:
I just checked in JShell and using `%s` directly worked. Will update.
```
jshell> String.format("Format Set<Integer> with o: %o", s);
| Exception java.util.IllegalFormatConversionException: o != java.util.ImmutableCollections$SetN
| at Formatter$FormatSpecifier.failConversion (Formatter.java:4426)
| at Formatter$FormatSpecifier.printInteger (Formatter.java:2938)
| at Formatter$FormatSpecifier.print (Formatter.java:2892)
| at Formatter.format (Formatter.java:2673)
| at Formatter.format (Formatter.java:2609)
| at String.format (String.java:2897)
| at (#7:1)
jshell> String.format("Format Set<Integer> with s: %s", s);
$10 ==> "Format Set<Integer> with s: [1, 2, 3]"
```
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876229162
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
Here's the associated class `UpdateRequirement.AsserrRefSnapshotId`: https://github.com/apache/iceberg/blob/3586e140dc3fbe0b0414464497be3ccd4ca0927d/core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java#L261-L272
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875231630
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -250,6 +251,8 @@ public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type, Intege
this.maxRefAgeMs = maxRefAgeMs;
}
+ // TODO - Consider renaming to `refName`, as the JSON is called `ref`. This also matches the equivalent
+ // UpdateRequirement.
public String name() {
Review Comment:
TODO / Possible Follow Up - Consider renaming to `refName`, as the JSON is called `ref`. This also matches the equivalent UpdateRequirement.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875309128
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,120 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
Review Comment:
I think it would be better to just embed `[ 2 ]` rather than relying on `Arrays.toString`.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876236295
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +379,55 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ // TODO - Return to this when FileIO isn't required.
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ // TODO - Instead of this we could just call removeProperties, which would avoid the need for null fields.
+ // but that's probably not intuitive for the user.
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
Review Comment:
+1. That's what I was trying to support, though as you mentioned people should be using `RemoveProperties`.
So this might be giving them just enough rope to hang themselves.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876235114
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(
+ refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
Review Comment:
No, they shouldn't be null. However, I could see somebody using this API to unset values given that it's referred to as `updated` in the field name.
Users should really use `RemoveProperties` for that purpose so happy to remove this to reduce complexity.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877540694
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
Review Comment:
Updated to use `%s` as demonstrated in jshell.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878355359
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -233,25 +233,25 @@ public void applyTo(TableMetadata.Builder metadataBuilder) {
}
class SetSnapshotRef implements MetadataUpdate {
- private final String name;
+ private final String refName;
private final Long snapshotId;
private final SnapshotRefType type;
private Integer minSnapshotsToKeep;
private Long maxSnapshotAgeMs;
private Long maxRefAgeMs;
- public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type, Integer minSnapshotsToKeep,
+ public SetSnapshotRef(String refName, Long snapshotId, SnapshotRefType type, Integer minSnapshotsToKeep,
Long maxSnapshotAgeMs, Long maxRefAgeMs) {
- this.name = name;
+ this.refName = refName;
this.snapshotId = snapshotId;
this.type = type;
this.minSnapshotsToKeep = minSnapshotsToKeep;
this.maxSnapshotAgeMs = maxSnapshotAgeMs;
this.maxRefAgeMs = maxRefAgeMs;
}
- public String name() {
- return name;
+ public String refName() {
Review Comment:
Reverted.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876240017
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(
+ refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
+
+ private static MetadataUpdate readRemoveProperties(JsonNode node) {
+ Preconditions.checkArgument(node.hasNonNull(REMOVED), "Missing field: removed");
+ JsonNode removedNode = node.get(REMOVED);
+ String[] removed = JsonUtil.getStringArray(removedNode);
Review Comment:
Yeah happy to add that. I might move some of these changes to another PR so that we can also add testing for `JsonUtil` class, but for now I'll continue to stage the work here (and possibly we'll leave it here).
--
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
[GitHub] [iceberg] rdblue merged pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4716:
URL: https://github.com/apache/iceberg/pull/4716
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875234623
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +379,55 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ // TODO - Return to this when FileIO isn't required.
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
Review Comment:
We'll need to return to this when `FileIO` can be plumbed through.
Happy to leave it out of this PR for now.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875236617
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +379,55 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ // TODO - Return to this when FileIO isn't required.
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ // TODO - Instead of this we could just call removeProperties, which would avoid the need for null fields.
+ // but that's probably not intuitive for the user.
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
Review Comment:
The introduction of `JsonUtil::getStringMapWithNullValues` was added to support setting properties to null.
This could also be achieved via `removeProperties`, but that might not be intuitive for the user.
Happy to split this part into a separate PR.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875285210
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
Review Comment:
Should I implement this in another PR instead? We still need to plumb through `FileIO` to `BaseSnapshot` so I think probably so.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875347352
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
Review Comment:
This seems to be working:
```
Preconditions.checkArgument(node.has(SNAPSHOT), "Cannot parse missing field: snapshot");
Snapshot snapshot = SnapshotParser.fromJson(null, node.get(SNAPSHOT));
```
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [WIP] [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r867033954
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +75,20 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
Review Comment:
This field is called `order-id` in the spec.
Due to discussion in #4693 , we've decided to align the names `order-id` and `write-order-id` on `sort-order-id` (and `default-sort-order-id`) to match the existing code.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875317767
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
+
+ private static void assertEqualsRemoveSnapshots(
+ MetadataUpdate.RemoveSnapshot expected, MetadataUpdate.RemoveSnapshot actual) {
+ Assert.assertEquals("Snapshots to remove should be the same", expected.snapshotId(), actual.snapshotId());
+ }
+
+ // See TODOs in MetadataUpdate.SetSnapshotRef.
+ private static void assertEqualsSetSnapshotRef(
+ MetadataUpdate.SetSnapshotRef expected, MetadataUpdate.SetSnapshotRef actual) {
+ Assert.fail("MetadataUpdate equality checking for SetSnapshotRef is not implemented yet");
+ }
Review Comment:
I don't see the note you're referring to. The parser looks like it supports this, so I think it can be implemented. This shouldn't be affected by whether or not the optional fields are serialized using `null` or are omitted. Though you will want tests for reading all cases (omitted, null, non-null) and make sure they match here.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876194991
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
I believe I copied this from another parser (which I don't believe we can't reuse without some refactoring as there's no ref `name` field in it). I'll check.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877541502
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(
+ refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
+
+ private static MetadataUpdate readRemoveProperties(JsonNode node) {
+ Preconditions.checkArgument(node.hasNonNull(REMOVED), "Missing field: removed");
+ JsonNode removedNode = node.get(REMOVED);
+ String[] removed = JsonUtil.getStringArray(removedNode);
Review Comment:
I added `getStringSet` and changed to use that. We should introduce a test class for `JsonUtil`, but I can take care of that in a follow up PR or open an issue for it as it could be a good issue for a new contributor.
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
Review Comment:
Updated as mentioned.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877540387
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
I updated this to `ref-name`, as well as updated the spec.
I also updated the code to use `refName` as the field in `SetSnapshotRef` as well as the accessor (to match `AssertRefSnapshotId`. Please let me know if you think the changes for the accessor are too much for this PR.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875241907
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
+
+ private static void assertEqualsRemoveSnapshots(
+ MetadataUpdate.RemoveSnapshot expected, MetadataUpdate.RemoveSnapshot actual) {
+ Assert.assertEquals("Snapshots to remove should be the same", expected.snapshotId(), actual.snapshotId());
+ }
+
+ // See TODOs in MetadataUpdate.SetSnapshotRef.
+ private static void assertEqualsSetSnapshotRef(
+ MetadataUpdate.SetSnapshotRef expected, MetadataUpdate.SetSnapshotRef actual) {
+ Assert.fail("MetadataUpdate equality checking for SetSnapshotRef is not implemented yet");
+ }
Review Comment:
See the notes in `MetadataUpdate.SetSnapshotRef` about possible things to update.
Happy to remove this from this PR and place it elsewhere.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875305790
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +379,55 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ // TODO - Return to this when FileIO isn't required.
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
+
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ private static MetadataUpdate readRemoveSnapshots(JsonNode node) {
+ Set<Long> snapshotIds = JsonUtil.getLongSetOrNull(SNAPSHOT_IDS, node);
+ Preconditions.checkArgument(snapshotIds != null && snapshotIds.size() == 1,
+ "Invalid set of snapshot ids to remove. Expected one value but received: %o", snapshotIds);
+ // TODO - Handle the difference between the spec having set semantics and the class having single value.
+ Long snapshotId = snapshotIds.iterator().next();
+ return new MetadataUpdate.RemoveSnapshot(snapshotId);
+ }
+
+ private static MetadataUpdate readSetSnapshotRef(JsonNode node) {
+ String refName = JsonUtil.getString(REF_NAME, node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
+ Integer minSnapshotsToKeep = JsonUtil.getIntOrNull(MIN_SNAPSHOTS_TO_KEEP, node);
+ Long maxSnapshotAgeMs = JsonUtil.getLongOrNull(MAX_SNAPSHOT_AGE_MS, node);
+ Long maxRefAgeMs = JsonUtil.getLongOrNull(MAX_REF_AGE_MS, node);
+ return new MetadataUpdate.SetSnapshotRef(refName, snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+
+ private static MetadataUpdate readSetProperties(JsonNode node) {
+ // TODO - Instead of this we could just call removeProperties, which would avoid the need for null fields.
+ // but that's probably not intuitive for the user.
+ Map<String, String> updated = JsonUtil.getStringMapWithNullValues(UPDATED, node);
+ return new MetadataUpdate.SetProperties(updated);
+ }
Review Comment:
Ah, I think the right way to remove properties at this level is to use the `RemovePropertiesUpdate` from the spec.
Otherwise, this is ambiguous because someone may be trying to set a property to null, instead of removing it. That's what I thought you were trying to support with my comment above. But I don't think that we allow setting properties to null.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875302458
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
I'd support updating this to `ref-name` here and in the spec.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878529564
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +379,55 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ // TODO - Return to this when FileIO isn't required.
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
+ return new MetadataUpdate.AddSnapshot(snapshot);
+ }
Review Comment:
We actually won't need to return to it for when FileIO is plumbed through for testing purposes, but we will want to consider it still in case.
I'm working on that PR now.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877519000
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
Updated to put as many on one line as possible.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875287975
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,57 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
Review Comment:
Same note about multiple snapshots being allowed in the code. Although for now this should be fine.
I'll hold off on editing this until more input re: following up `RemoveSnapshots` in another PR or simply only supporting one value is determined.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875300775
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -234,6 +290,58 @@ private static void writeAddSortOrder(MetadataUpdate.AddSortOrder update, JsonGe
SortOrderParser.toJson(update.sortOrder(), gen);
}
+ private static void writeSetDefaultSortOrder(MetadataUpdate.SetDefaultSortOrder update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SORT_ORDER_ID, update.sortOrderId());
+ }
+
+ private static void writeAddSnapshot(MetadataUpdate.AddSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SNAPSHOT);
+ SnapshotParser.toJson(update.snapshot(), gen);
+ }
+
+ // TODO - Reconcile the spec's set-based removal with the current class implementation that only handles one value.
+ private static void writeRemoveSnapshots(MetadataUpdate.RemoveSnapshot update, JsonGenerator gen) throws IOException {
+ gen.writeArrayFieldStart(SNAPSHOT_IDS);
+ for (long snapshotId : ImmutableSet.of(update.snapshotId())) {
+ gen.writeNumber(snapshotId);
+ }
+ gen.writeEndArray();
+ }
+
+ private static void writeSetSnapshotRef(MetadataUpdate.SetSnapshotRef update, JsonGenerator gen) throws IOException {
+ gen.writeStringField(REF_NAME, update.name());
+ gen.writeNumberField(SNAPSHOT_ID, update.snapshotId());
+ gen.writeStringField(TYPE, update.type());
+ // TODO - Should we explicitly set the null values?
+ JsonUtil.writeIntegerFieldIf(update.minSnapshotsToKeep() != null,
+ MIN_SNAPSHOTS_TO_KEEP, update.minSnapshotsToKeep(), gen);
+ JsonUtil.writeLongFieldIf(update.maxSnapshotAgeMs() != null,
+ MAX_SNAPSHOT_AGE_MS, update.maxSnapshotAgeMs(), gen);
+ JsonUtil.writeLongFieldIf(update.maxRefAgeMs() != null, MAX_REF_AGE_MS,
+ update.maxRefAgeMs(), gen);
Review Comment:
I think they should be omitted.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875298918
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +502,52 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
Review Comment:
Accidental whitespace 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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875307247
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,122 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
Review Comment:
No, I don't think that the `FileIO` should not affect the test done here. We just need to make sure that the `Snapshot` info matches.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r875346828
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -271,5 +378,51 @@ private static MetadataUpdate readAddSortOrder(JsonNode node) {
UnboundSortOrder sortOrder = SortOrderParser.fromJson(sortOrderNode);
return new MetadataUpdate.AddSortOrder(sortOrder);
}
+
+ private static MetadataUpdate readSetDefaultSortOrder(JsonNode node) {
+ int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
+ return new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ }
+
+ private static MetadataUpdate readAddSnapshot(JsonNode node) {
+ Snapshot snapshot = SnapshotParser.fromJson(null, node);
Review Comment:
Looks like this is slightly wrong. The node passed in is the update, but it should pass `node.get(SNAPSHOT)` here instead.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876229162
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
Here's the associated class `UpdateRequirement.AsserrRefSnapshotId`: https://github.com/apache/iceberg/blob/3586e140dc3fbe0b0414464497be3ccd4ca0927d/core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java#L261-L297
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876248205
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -251,11 +274,120 @@ public void testAddSortOrderFromJson() {
assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
}
+ @Test
+ public void testSetDefaultSortOrderToJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String expected = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default sort order should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultSortOrderFromJson() {
+ String action = MetadataUpdateParser.SET_DEFAULT_SORT_ORDER;
+ int sortOrderId = 2;
+ String json = String.format("{\"action\":\"%s\",\"sort-order-id\":%d}", action, sortOrderId);
+ MetadataUpdate expected = new MetadataUpdate.SetDefaultSortOrder(sortOrderId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - AddSnapshot tests.
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsFromJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
+ MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotIds[0]);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ // TODO - Tests that handle multiple snapshot ids.
+ @Test
+ public void testRemoveSnapshotsToJson() {
+ String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
+ long[] snapshotIds = { 2L };
+ String expected = String.format("{\"action\":\"%s\",\"snapshot-ids\":%s}", action, Arrays.toString(snapshotIds));
Review Comment:
Yeah good call 👍
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r877526180
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -350,9 +485,51 @@ private static void assertEqualsAddSortOrder(
UnboundSortOrder.UnboundSortField actualField = actual.sortOrder().fields().get(i);
Assert.assertTrue("Fields of the sort order should be the same",
expectedField.sourceId() == actualField.sourceId() &&
- expectedField.nullOrder().equals(actualField.nullOrder()) &&
- expectedField.direction().equals(actualField.direction()) &&
- Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
+ expectedField.nullOrder().equals(actualField.nullOrder()) &&
+ expectedField.direction().equals(actualField.direction()) &&
+ Objects.equals(expectedField.transformAsString(), actualField.transformAsString()));
});
}
+
+ private static void assertEqualsSetDefaultSortOrder(
+ MetadataUpdate.SetDefaultSortOrder expected, MetadataUpdate.SetDefaultSortOrder actual) {
+ Assert.assertEquals("Sort order id should be the same", expected.sortOrderId(), actual.sortOrderId());
+ }
+
+ // TODO - Come back to this when FileIO is injected. If this class extends TableTestBase, the validateSnapshot
+ // methods can be reused. Also, this needs to handle V1 vs V2 for assertions. Or casting to BaseSnapshot,
+ // BaseSnapshot::equals can be used.
+ private static void assertEqualsAddSnapshot(
+ MetadataUpdate.AddSnapshot expected, MetadataUpdate.AddSnapshot actual) {
+ Assert.fail("MetadataUpdate equality checking for AddSnapshot is not implemented yet");
+ }
Review Comment:
I think that's sufficient.
What about the snapshots schema id, operation, and the timestamp? Possibly overkill as the three you mentioned (especially the combination of snapshot ID and manifest list location) could arguably be considered sufficient.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r878356165
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -250,6 +251,8 @@ public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type, Intege
this.maxRefAgeMs = maxRefAgeMs;
}
+ // TODO - Consider renaming to `refName`, as the JSON is called `ref`. This also matches the equivalent
+ // UpdateRequirement.
public String name() {
Review Comment:
Change tot he code has been reverted.
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #4716: [CORE] - Add remaining parsers for MetadataUpdate implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4716:
URL: https://github.com/apache/iceberg/pull/4716#discussion_r876229162
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -72,6 +78,32 @@ private MetadataUpdateParser() {
// AddSortOrder
private static final String SORT_ORDER = "sort-order";
+ // SetDefaultSortOrder
+ private static final String SORT_ORDER_ID = "sort-order-id";
+
+ // AddSnapshot
+ private static final String SNAPSHOT = "snapshot";
+
+ // RemoveSnapshot
+ private static final String SNAPSHOT_IDS = "snapshot-ids";
+
+ // SetSnapshotRef
+ private static final String REF_NAME = "ref";
Review Comment:
Here's the associated class `UpdateRequirement.AssertRefSnapshotId`: https://github.com/apache/iceberg/blob/3586e140dc3fbe0b0414464497be3ccd4ca0927d/core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java#L261-L297
--
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