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/04/26 06:15:49 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4632: Core - Implement Parser for more MetadataUpdate subtypes
kbendick opened a new pull request, #4632:
URL: https://github.com/apache/iceberg/pull/4632
This covers several more parsers for MetadataUpdate
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860139741
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
throws IOException {
gen.writeNumberField(FORMAT_VERSION, update.formatVersion());
}
+ private static void writeAsAddSchema(MetadataUpdate.AddSchema update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SCHEMA);
+ SchemaParser.toJson(update.schema(), gen);
+ gen.writeNumberField(LAST_COLUMN_ID, update.lastColumnId());
+ }
+
+ private static void writeAsSetCurrentSchema(MetadataUpdate.SetCurrentSchema update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SCHEMA_ID, update.schemaId());
+ }
+
+ private static void writeAsSetDefaultPartitionSpec(MetadataUpdate.SetDefaultPartitionSpec update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SPEC_ID, update.specId());
+ }
+
private static MetadataUpdate readAsUpgradeFormatVersion(JsonNode node) {
int formatVersion = JsonUtil.getInt(FORMAT_VERSION, node);
return new MetadataUpdate.UpgradeFormatVersion(formatVersion);
}
+
+ private static MetadataUpdate readAsAddSchema(JsonNode node) {
+ Preconditions.checkArgument(node.hasNonNull(SCHEMA),
+ "Cannot parse missing field: schema");
+ JsonNode schemaNode = node.get(SCHEMA);
+ Preconditions.checkArgument(schemaNode.isObject(),
+ "Invalid type for schema field. Expected object");
Review Comment:
I think this is already checked in `SchemaParser.fromJson`. It reads the type and then makes sure it is a struct, which should provide 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] kbendick commented on a diff in pull request #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r859057287
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
Review Comment:
Updated the function name to `writeAs*` to match the existing `readAs*`.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r858325088
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -96,17 +109,42 @@ public static String toJson(MetadataUpdate metadataUpdate, boolean pretty) {
public static void toJson(MetadataUpdate metadataUpdate, JsonGenerator generator) throws IOException {
String updateAction = ACTIONS.get(metadataUpdate.getClass());
- // Add shared `action` field used to differentiate between implementations of MetadataUpdate
generator.writeStartObject();
generator.writeStringField(ACTION, updateAction);
- // Write subclass specific fields
switch (updateAction) {
case ASSIGN_UUID:
throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for AssignUUID");
case UPGRADE_FORMAT_VERSION:
- writeUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ writeAsUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ break;
+ case ADD_SCHEMA:
+ writeAsAddSchema((MetadataUpdate.AddSchema) metadataUpdate, generator);
+ break;
+ case SET_CURRENT_SCHEMA:
+ writeAsSetCurrentSchema((MetadataUpdate.SetCurrentSchema) metadataUpdate, generator);
break;
+ case SET_DEFAULT_PARTITION_SPEC:
+ writeAsSetDefaultPartitionSpec((MetadataUpdate.SetDefaultPartitionSpec) metadataUpdate, generator);
+ break;
+ case ADD_PARTITION_SPEC:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
+ case ADD_SORT_ORDER:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
Review Comment:
Originally this used case fall through to get the process of listing all cases into the source code, but error prone complains when switch cases fall through.
Same for the other 2 locations.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r859345921
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
Review Comment:
So I had the same thought, but in the spec, the action is defined as `remove-snapshots`.
If you look at the implementation, it immediately converts the single snapshot ID to a set (which is what the spec defines).
So it might make more sense to eventually rename the class `RemoveSnapshots` and/or add a constructor that takes in a collection / iterable. That's what I intend to do in one of my follow up PRs that implements the serde for this class.
Good catch 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] rdblue commented on a diff in pull request #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860140891
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -21,11 +21,33 @@
import java.util.List;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;
public class TestMetadataUpdateParser {
+ // MetadataUpdate actions
Review Comment:
Can you make the other references package-private so you don't have to copy them? Or is this for testing purposes so that you ensure you're not altering tests by altering the constants? (I probably wouldn't worry about that)
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860137320
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
Review Comment:
I think it would also be fine to drop `As` from the read side if you want them to match. `readAddSchema` makes sense to me.
--
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 #4632: Core - Implement Parser for more MetadataUpdate subtypes
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#issuecomment-1109399253
cc @rdblue
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860141804
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -60,15 +82,119 @@ public void testUpgradeFormatVersionFromJson() {
expected, MetadataUpdateParser.toJson(actual));
}
+ @Test
+ public void testAddSchemaFromJson() {
+ String action = "add-schema";
+ Schema schema = ID_DATA_SCHEMA;
+ int lastColumnId = schema.highestFieldId();
+ String json = String.format("{\"action\":\"add-schema\",\"schema\":%s,\"last-column-id\":%d}",
+ SchemaParser.toJson(schema), lastColumnId);
+ MetadataUpdate actualUpdate = new MetadataUpdate.AddSchema(schema, lastColumnId);
+ assertEquals(action, actualUpdate, MetadataUpdateParser.fromJson(json));
+ }
+
+ @Test
+ public void testAddSchemaToJson() {
+ Schema schema = ID_DATA_SCHEMA;
+ int lastColumnId = schema.highestFieldId();
+ String expected = String.format("{\"action\":\"add-schema\",\"schema\":%s,\"last-column-id\":%d}",
+ SchemaParser.toJson(schema), lastColumnId);
+ MetadataUpdate update = new MetadataUpdate.AddSchema(schema, lastColumnId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Add schema should convert to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetCurrentSchemaFromJson() {
+ String action = SET_CURRENT_SCHEMA;
+ int schemaId = 6;
+ String json = String.format("{\"action\":\"%s\",\"schema-id\":%d}", action, schemaId);
+ MetadataUpdate.SetCurrentSchema expected = new MetadataUpdate.SetCurrentSchema(schemaId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ @Test
+ public void testSetCurrentSchemaToJson() {
+ String action = SET_CURRENT_SCHEMA;
+ int schemaId = 6;
+ String expected = String.format("{\"action\":\"%s\",\"schema-id\":%d}", action, schemaId);
+ MetadataUpdate update = new MetadataUpdate.SetCurrentSchema(schemaId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set current schema should convert to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultPartitionSpecToJson() {
+ String action = SET_DEFAULT_PARTITION_SPEC;
+ int specId = 4;
+ String expected = String.format("{\"action\":\"%s\",\"spec-id\":%d}", action, specId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultPartitionSpec(specId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default partition spec should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultPartitionSpecFromJson() {
+ String action = SET_DEFAULT_PARTITION_SPEC;
+ int specId = 4;
+ String json = String.format("{\"action\":\"%s\",\"spec-id\":%d}", action, specId);
+ MetadataUpdate.SetDefaultPartitionSpec expected = new MetadataUpdate.SetDefaultPartitionSpec(specId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
public void assertEquals(String action, MetadataUpdate expectedUpdate, MetadataUpdate actualUpdate) {
Review Comment:
This looks reasonable to me.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860145232
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
throws IOException {
gen.writeNumberField(FORMAT_VERSION, update.formatVersion());
}
+ private static void writeAsAddSchema(MetadataUpdate.AddSchema update, JsonGenerator gen) throws IOException {
+ gen.writeFieldName(SCHEMA);
+ SchemaParser.toJson(update.schema(), gen);
+ gen.writeNumberField(LAST_COLUMN_ID, update.lastColumnId());
+ }
+
+ private static void writeAsSetCurrentSchema(MetadataUpdate.SetCurrentSchema update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SCHEMA_ID, update.schemaId());
+ }
+
+ private static void writeAsSetDefaultPartitionSpec(MetadataUpdate.SetDefaultPartitionSpec update, JsonGenerator gen)
+ throws IOException {
+ gen.writeNumberField(SPEC_ID, update.specId());
+ }
+
private static MetadataUpdate readAsUpgradeFormatVersion(JsonNode node) {
int formatVersion = JsonUtil.getInt(FORMAT_VERSION, node);
return new MetadataUpdate.UpgradeFormatVersion(formatVersion);
}
+
+ private static MetadataUpdate readAsAddSchema(JsonNode node) {
+ Preconditions.checkArgument(node.hasNonNull(SCHEMA),
+ "Cannot parse missing field: schema");
+ JsonNode schemaNode = node.get(SCHEMA);
+ Preconditions.checkArgument(schemaNode.isObject(),
+ "Invalid type for schema field. Expected object");
Review Comment:
Ah good point. I'll defer to the parser for consistency.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860144484
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -56,6 +56,19 @@ private MetadataUpdateParser() {
// UpgradeFormatVersion
private static final String FORMAT_VERSION = "format-version";
+ // AddSchema
+ private static final String SCHEMA = "schema";
+ private static final String LAST_COLUMN_ID = "last-column-id";
+
+ // SetCurrentSchema
+ private static final String SCHEMA_ID = "schema-id";
+
+ // AddPartitionSpec
+ private static final String SPEC = "spec";
Review Comment:
Originally I had it in this PR, but I realized I had some concerns around `fieldId`, so I took it out to allow that conversation to be had in a more isolated place.
Seems I forgot to remove its fields. My bad!
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4632:
URL: https://github.com/apache/iceberg/pull/4632
--
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 #4632: Core - Implement Parser for more MetadataUpdate subtypes
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#issuecomment-1109394145
I left `AddPartitionSpec` unimplemented because of concerns about processing incoming JSON that does not have field-id set when parsed, which would be assigned default field ids (starting at 1000). Want to double check any potential consequences or realities of that happening.
I'm not even sure that could happen / needs to be accounted for, but after having that thought I figured it would be better to open the `AddPartitionSpec` SetDe in a separate PR given that smaller PRs are generally better anyway. I'll open that one shortly.
--
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] samredai commented on a diff in pull request #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r859156110
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
Review Comment:
Since the class is `RemoveSnapshot`, should we make this singular, meaning `REMOVE_SNAPSHOT`? This is a nit but all of the other actions seem to match the class name.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860144841
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
Review Comment:
Ok. I'll drop `As` from the read side. Thanks for the feedback.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860146063
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -21,11 +21,33 @@
import java.util.List;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;
public class TestMetadataUpdateParser {
+ // MetadataUpdate actions
Review Comment:
package-private would be fine. The consideration around not altering constants was not something I'd thought of.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860135116
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
+ case SET_SNAPSHOT_REF:
+ case SET_PROPERTIES:
+ case REMOVE_PROPERTIES:
+ case SET_LOCATION:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdatefromJson for " + action);
default:
throw new UnsupportedOperationException(
String.format("Cannot convert metadata update action to json: %s", action));
}
}
- // Write all fields specific to UpgradeFormatVersion
- private static void writeUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
+ private static void writeAsUpgradeFormatVersion(MetadataUpdate.UpgradeFormatVersion update, JsonGenerator gen)
Review Comment:
I don't think the names make sense with `As`. I think it's okay if it doesn't quite match. I'd opt for names that you can read easily.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r858325088
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -96,17 +109,42 @@ public static String toJson(MetadataUpdate metadataUpdate, boolean pretty) {
public static void toJson(MetadataUpdate metadataUpdate, JsonGenerator generator) throws IOException {
String updateAction = ACTIONS.get(metadataUpdate.getClass());
- // Add shared `action` field used to differentiate between implementations of MetadataUpdate
generator.writeStartObject();
generator.writeStringField(ACTION, updateAction);
- // Write subclass specific fields
switch (updateAction) {
case ASSIGN_UUID:
throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for AssignUUID");
case UPGRADE_FORMAT_VERSION:
- writeUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ writeAsUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ break;
+ case ADD_SCHEMA:
+ writeAsAddSchema((MetadataUpdate.AddSchema) metadataUpdate, generator);
+ break;
+ case SET_CURRENT_SCHEMA:
+ writeAsSetCurrentSchema((MetadataUpdate.SetCurrentSchema) metadataUpdate, generator);
break;
+ case SET_DEFAULT_PARTITION_SPEC:
+ writeAsSetDefaultPartitionSpec((MetadataUpdate.SetDefaultPartitionSpec) metadataUpdate, generator);
+ break;
+ case ADD_PARTITION_SPEC:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
+ case ADD_SORT_ORDER:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
Review Comment:
Originally this used case fall through to get the process of listing all cases into the source code, but error prone complains when switch cases fall through.
Same for the other 2 locations.
--
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 #4632: Core - Implement Parser for more MetadataUpdate subtypes
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r858325088
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -96,17 +109,42 @@ public static String toJson(MetadataUpdate metadataUpdate, boolean pretty) {
public static void toJson(MetadataUpdate metadataUpdate, JsonGenerator generator) throws IOException {
String updateAction = ACTIONS.get(metadataUpdate.getClass());
- // Add shared `action` field used to differentiate between implementations of MetadataUpdate
generator.writeStartObject();
generator.writeStringField(ACTION, updateAction);
- // Write subclass specific fields
switch (updateAction) {
case ASSIGN_UUID:
throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for AssignUUID");
case UPGRADE_FORMAT_VERSION:
- writeUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ writeAsUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ break;
+ case ADD_SCHEMA:
+ writeAsAddSchema((MetadataUpdate.AddSchema) metadataUpdate, generator);
+ break;
+ case SET_CURRENT_SCHEMA:
+ writeAsSetCurrentSchema((MetadataUpdate.SetCurrentSchema) metadataUpdate, generator);
break;
+ case SET_DEFAULT_PARTITION_SPEC:
+ writeAsSetDefaultPartitionSpec((MetadataUpdate.SetDefaultPartitionSpec) metadataUpdate, generator);
+ break;
+ case ADD_PARTITION_SPEC:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
+ case ADD_SORT_ORDER:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
Review Comment:
Originally this used case fall through to get the process of listing all cases into the source code, but error prone complains when switch cases fall through (rightly so).
Same for the other 2 locations.
--
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 #4632: Core - Implement Parser for more MetadataUpdate subtypes
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r858325088
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -96,17 +109,42 @@ public static String toJson(MetadataUpdate metadataUpdate, boolean pretty) {
public static void toJson(MetadataUpdate metadataUpdate, JsonGenerator generator) throws IOException {
String updateAction = ACTIONS.get(metadataUpdate.getClass());
- // Add shared `action` field used to differentiate between implementations of MetadataUpdate
generator.writeStartObject();
generator.writeStringField(ACTION, updateAction);
- // Write subclass specific fields
switch (updateAction) {
case ASSIGN_UUID:
throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for AssignUUID");
case UPGRADE_FORMAT_VERSION:
- writeUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ writeAsUpgradeFormatVersion((MetadataUpdate.UpgradeFormatVersion) metadataUpdate, generator);
+ break;
+ case ADD_SCHEMA:
+ writeAsAddSchema((MetadataUpdate.AddSchema) metadataUpdate, generator);
+ break;
+ case SET_CURRENT_SCHEMA:
+ writeAsSetCurrentSchema((MetadataUpdate.SetCurrentSchema) metadataUpdate, generator);
break;
+ case SET_DEFAULT_PARTITION_SPEC:
+ writeAsSetDefaultPartitionSpec((MetadataUpdate.SetDefaultPartitionSpec) metadataUpdate, generator);
+ break;
+ case ADD_PARTITION_SPEC:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
+ case ADD_SORT_ORDER:
+ throw new UnsupportedOperationException("Not Implemented: MetadataUpdate#toJson for " + updateAction);
Review Comment:
Originally this used case fall through to get the process of listing all cases into the source code, but error prone complains when switch cases fall through (rightly 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] samredai commented on a diff in pull request #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r859156110
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -140,21 +170,73 @@ public static MetadataUpdate fromJson(JsonNode jsonNode) {
throw new UnsupportedOperationException("Not implemented: AssignUUID");
case UPGRADE_FORMAT_VERSION:
return readAsUpgradeFormatVersion(jsonNode);
+ case ADD_SCHEMA:
+ return readAsAddSchema(jsonNode);
+ case SET_CURRENT_SCHEMA:
+ return readAsSetCurrentSchema(jsonNode);
+ case SET_DEFAULT_PARTITION_SPEC:
+ return readAsSetDefaultPartitionSpec(jsonNode);
+ case ADD_PARTITION_SPEC:
+ case ADD_SORT_ORDER:
+ case SET_DEFAULT_SORT_ORDER:
+ case ADD_SNAPSHOT:
+ case REMOVE_SNAPSHOTS:
Review Comment:
Since the class is `RemoveSnapshot`, should we make this singular, meaning `REMOVE_SNAPSHOT`. This is a nit but all of the other actions seem to match the class name.
--
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 #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r859059722
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -60,15 +82,119 @@ public void testUpgradeFormatVersionFromJson() {
expected, MetadataUpdateParser.toJson(actual));
}
+ @Test
+ public void testAddSchemaFromJson() {
+ String action = "add-schema";
+ Schema schema = ID_DATA_SCHEMA;
+ int lastColumnId = schema.highestFieldId();
+ String json = String.format("{\"action\":\"add-schema\",\"schema\":%s,\"last-column-id\":%d}",
+ SchemaParser.toJson(schema), lastColumnId);
+ MetadataUpdate actualUpdate = new MetadataUpdate.AddSchema(schema, lastColumnId);
+ assertEquals(action, actualUpdate, MetadataUpdateParser.fromJson(json));
+ }
+
+ @Test
+ public void testAddSchemaToJson() {
+ Schema schema = ID_DATA_SCHEMA;
+ int lastColumnId = schema.highestFieldId();
+ String expected = String.format("{\"action\":\"add-schema\",\"schema\":%s,\"last-column-id\":%d}",
+ SchemaParser.toJson(schema), lastColumnId);
+ MetadataUpdate update = new MetadataUpdate.AddSchema(schema, lastColumnId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Add schema should convert to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetCurrentSchemaFromJson() {
+ String action = SET_CURRENT_SCHEMA;
+ int schemaId = 6;
+ String json = String.format("{\"action\":\"%s\",\"schema-id\":%d}", action, schemaId);
+ MetadataUpdate.SetCurrentSchema expected = new MetadataUpdate.SetCurrentSchema(schemaId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
+ @Test
+ public void testSetCurrentSchemaToJson() {
+ String action = SET_CURRENT_SCHEMA;
+ int schemaId = 6;
+ String expected = String.format("{\"action\":\"%s\",\"schema-id\":%d}", action, schemaId);
+ MetadataUpdate update = new MetadataUpdate.SetCurrentSchema(schemaId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set current schema should convert to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultPartitionSpecToJson() {
+ String action = SET_DEFAULT_PARTITION_SPEC;
+ int specId = 4;
+ String expected = String.format("{\"action\":\"%s\",\"spec-id\":%d}", action, specId);
+ MetadataUpdate update = new MetadataUpdate.SetDefaultPartitionSpec(specId);
+ String actual = MetadataUpdateParser.toJson(update);
+ Assert.assertEquals("Set default partition spec should serialize to the correct JSON value", expected, actual);
+ }
+
+ @Test
+ public void testSetDefaultPartitionSpecFromJson() {
+ String action = SET_DEFAULT_PARTITION_SPEC;
+ int specId = 4;
+ String json = String.format("{\"action\":\"%s\",\"spec-id\":%d}", action, specId);
+ MetadataUpdate.SetDefaultPartitionSpec expected = new MetadataUpdate.SetDefaultPartitionSpec(specId);
+ assertEquals(action, expected, MetadataUpdateParser.fromJson(json));
+ }
+
public void assertEquals(String action, MetadataUpdate expectedUpdate, MetadataUpdate actualUpdate) {
Review Comment:
We could just have each unit test call the correct `assertEquals` function based on the type.
However, this tests comparing two `MetadataUpdate` objects first and foremost. As we'll mostly be working with `MetadataUpdate` and not the specific subtype, I went with this.
I am open to suggestions 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 commented on a diff in pull request #4632: Core - Implement MetadataUpdateParser for AddSchema, SetCurrentSchema, and SetDefaultPartitionSpec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4632:
URL: https://github.com/apache/iceberg/pull/4632#discussion_r860137726
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -56,6 +56,19 @@ private MetadataUpdateParser() {
// UpgradeFormatVersion
private static final String FORMAT_VERSION = "format-version";
+ // AddSchema
+ private static final String SCHEMA = "schema";
+ private static final String LAST_COLUMN_ID = "last-column-id";
+
+ // SetCurrentSchema
+ private static final String SCHEMA_ID = "schema-id";
+
+ // AddPartitionSpec
+ private static final String SPEC = "spec";
Review Comment:
Did you intend to add `AddPartitionSpec` to 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