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 2020/10/14 22:46:20 UTC
[GitHub] [iceberg] yyanyy opened a new pull request #1614: Core: ensure sort order and specs exist in v2 when parsing table metadata (#1419)
yyanyy opened a new pull request #1614:
URL: https://github.com/apache/iceberg/pull/1614
For tests, created a new folder `testfiles` to store json strings used for testing `TableMetadataParser.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.
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] yyanyy commented on a change in pull request #1614: Core: ensure sort order and specs exist in v2 when parsing table metadata
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #1614:
URL: https://github.com/apache/iceberg/pull/1614#discussion_r513816527
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -446,68 +448,48 @@ public void testVersionValidation() {
@Test
public void testParserVersionValidation() throws Exception {
- String supportedVersion = toJsonWithVersion(
- TableMetadata.newTableMetadata(TEST_SCHEMA, SPEC_5, TEST_LOCATION, ImmutableMap.of()),
- TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION);
- TableMetadata parsed = TableMetadataParser.fromJson(
- ops.io(), null, JsonUtil.mapper().readValue(supportedVersion, JsonNode.class));
- Assert.assertNotNull("Should successfully read supported metadata version", parsed);
-
- String unsupportedVersion = toJsonWithVersion(
- TableMetadata.newTableMetadata(TEST_SCHEMA, SPEC_5, TEST_LOCATION, ImmutableMap.of()),
- TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION + 1);
+ String supportedVersion1 = readTableMetadataInputFile("TableMetadataV1Valid.json");
+ TableMetadata parsed1 = TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(supportedVersion1, JsonNode.class));
+ Assert.assertNotNull("Should successfully read supported metadata version", parsed1);
+
+ String supportedVersion2 = readTableMetadataInputFile("TableMetadataV2Valid.json");
+ TableMetadata parsed2 = TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(supportedVersion2, JsonNode.class));
+ Assert.assertNotNull("Should successfully read supported metadata version", parsed2);
+
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataUnsupportedVersion.json");
AssertHelpers.assertThrows("Should not read unsupported metadata",
IllegalArgumentException.class, "Cannot read unsupported version",
() -> TableMetadataParser.fromJson(
- ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class)));
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
}
- public static String toJsonWithVersion(TableMetadata metadata, int version) {
- StringWriter writer = new StringWriter();
- try {
- JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
-
- generator.writeStartObject(); // start table metadata object
-
- generator.writeNumberField(FORMAT_VERSION, version);
- generator.writeStringField(TABLE_UUID, metadata.uuid());
- generator.writeStringField(LOCATION, metadata.location());
- generator.writeNumberField(LAST_UPDATED_MILLIS, metadata.lastUpdatedMillis());
- if (version > 1) {
- generator.writeNumberField(TableMetadataParser.LAST_SEQUENCE_NUMBER, metadata.lastSequenceNumber());
- }
- generator.writeNumberField(LAST_COLUMN_ID, metadata.lastColumnId());
- generator.writeFieldName(SCHEMA);
- SchemaParser.toJson(metadata.schema(), generator);
-
- // mimic an old writer by writing only partition-spec and not the default ID or spec list
- generator.writeFieldName(PARTITION_SPEC);
- PartitionSpecParser.toJsonFields(metadata.spec(), generator);
-
- generator.writeObjectFieldStart(PROPERTIES);
- for (Map.Entry<String, String> keyValue : metadata.properties().entrySet()) {
- generator.writeStringField(keyValue.getKey(), keyValue.getValue());
- }
- generator.writeEndObject();
-
- generator.writeNumberField(CURRENT_SNAPSHOT_ID,
- metadata.currentSnapshot() != null ? metadata.currentSnapshot().snapshotId() : -1);
-
- generator.writeArrayFieldStart(SNAPSHOTS);
- for (Snapshot snapshot : metadata.snapshots()) {
- SnapshotParser.toJson(snapshot, generator);
- }
- generator.writeEndArray();
- // skip the snapshot log
+ @Test
+ public void testParserV2PartitionSpecsValidation() throws Exception {
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataV2MissingPartitionSpecs.json");
+ AssertHelpers.assertThrows("Should reject v2 metadata without partition specs",
+ IllegalArgumentException.class, "partition-specs must exist in format v2",
+ () -> TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
+ }
- generator.writeEndObject(); // end table metadata object
+ @Test
+ public void testParserV2SortOrderValidation() throws Exception {
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataV2MissingSortOrder.json");
+ AssertHelpers.assertThrows("Should reject v2 metadata without sort order",
+ IllegalArgumentException.class, "sort-orders must exist in format v2",
+ () -> TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
+ }
- generator.flush();
- } catch (IOException e) {
- throw new UncheckedIOException(String.format("Failed to write json for: %s", metadata), e);
- }
- return writer.toString();
+ private String readTableMetadataInputFile(String fileName) throws IOException {
+ Path path = Paths.get(new File("").getAbsolutePath(), "src/test/testfiles", fileName);
Review comment:
Thank you for the review! Using `ClassLoader.getResource` is definitely cleaner. Updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1614: Core: ensure sort order and specs exist in v2 when parsing table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1614:
URL: https://github.com/apache/iceberg/pull/1614#issuecomment-718105613
Looks good to me, other than assuming the working directory for the build. Could you update the test to find files in `src/test/resources` 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.
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 change in pull request #1614: Core: ensure sort order and specs exist in v2 when parsing table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1614:
URL: https://github.com/apache/iceberg/pull/1614#discussion_r513647378
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -446,68 +448,48 @@ public void testVersionValidation() {
@Test
public void testParserVersionValidation() throws Exception {
- String supportedVersion = toJsonWithVersion(
- TableMetadata.newTableMetadata(TEST_SCHEMA, SPEC_5, TEST_LOCATION, ImmutableMap.of()),
- TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION);
- TableMetadata parsed = TableMetadataParser.fromJson(
- ops.io(), null, JsonUtil.mapper().readValue(supportedVersion, JsonNode.class));
- Assert.assertNotNull("Should successfully read supported metadata version", parsed);
-
- String unsupportedVersion = toJsonWithVersion(
- TableMetadata.newTableMetadata(TEST_SCHEMA, SPEC_5, TEST_LOCATION, ImmutableMap.of()),
- TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION + 1);
+ String supportedVersion1 = readTableMetadataInputFile("TableMetadataV1Valid.json");
+ TableMetadata parsed1 = TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(supportedVersion1, JsonNode.class));
+ Assert.assertNotNull("Should successfully read supported metadata version", parsed1);
+
+ String supportedVersion2 = readTableMetadataInputFile("TableMetadataV2Valid.json");
+ TableMetadata parsed2 = TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(supportedVersion2, JsonNode.class));
+ Assert.assertNotNull("Should successfully read supported metadata version", parsed2);
+
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataUnsupportedVersion.json");
AssertHelpers.assertThrows("Should not read unsupported metadata",
IllegalArgumentException.class, "Cannot read unsupported version",
() -> TableMetadataParser.fromJson(
- ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class)));
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
}
- public static String toJsonWithVersion(TableMetadata metadata, int version) {
- StringWriter writer = new StringWriter();
- try {
- JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
-
- generator.writeStartObject(); // start table metadata object
-
- generator.writeNumberField(FORMAT_VERSION, version);
- generator.writeStringField(TABLE_UUID, metadata.uuid());
- generator.writeStringField(LOCATION, metadata.location());
- generator.writeNumberField(LAST_UPDATED_MILLIS, metadata.lastUpdatedMillis());
- if (version > 1) {
- generator.writeNumberField(TableMetadataParser.LAST_SEQUENCE_NUMBER, metadata.lastSequenceNumber());
- }
- generator.writeNumberField(LAST_COLUMN_ID, metadata.lastColumnId());
- generator.writeFieldName(SCHEMA);
- SchemaParser.toJson(metadata.schema(), generator);
-
- // mimic an old writer by writing only partition-spec and not the default ID or spec list
- generator.writeFieldName(PARTITION_SPEC);
- PartitionSpecParser.toJsonFields(metadata.spec(), generator);
-
- generator.writeObjectFieldStart(PROPERTIES);
- for (Map.Entry<String, String> keyValue : metadata.properties().entrySet()) {
- generator.writeStringField(keyValue.getKey(), keyValue.getValue());
- }
- generator.writeEndObject();
-
- generator.writeNumberField(CURRENT_SNAPSHOT_ID,
- metadata.currentSnapshot() != null ? metadata.currentSnapshot().snapshotId() : -1);
-
- generator.writeArrayFieldStart(SNAPSHOTS);
- for (Snapshot snapshot : metadata.snapshots()) {
- SnapshotParser.toJson(snapshot, generator);
- }
- generator.writeEndArray();
- // skip the snapshot log
+ @Test
+ public void testParserV2PartitionSpecsValidation() throws Exception {
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataV2MissingPartitionSpecs.json");
+ AssertHelpers.assertThrows("Should reject v2 metadata without partition specs",
+ IllegalArgumentException.class, "partition-specs must exist in format v2",
+ () -> TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
+ }
- generator.writeEndObject(); // end table metadata object
+ @Test
+ public void testParserV2SortOrderValidation() throws Exception {
+ String unsupportedVersion = readTableMetadataInputFile("TableMetadataV2MissingSortOrder.json");
+ AssertHelpers.assertThrows("Should reject v2 metadata without sort order",
+ IllegalArgumentException.class, "sort-orders must exist in format v2",
+ () -> TableMetadataParser.fromJson(
+ ops.io(), null, JsonUtil.mapper().readValue(unsupportedVersion, JsonNode.class))
+ );
+ }
- generator.flush();
- } catch (IOException e) {
- throw new UncheckedIOException(String.format("Failed to write json for: %s", metadata), e);
- }
- return writer.toString();
+ private String readTableMetadataInputFile(String fileName) throws IOException {
+ Path path = Paths.get(new File("").getAbsolutePath(), "src/test/testfiles", fileName);
Review comment:
I think the test files should be in `src/test/resources` so that `ClassLoader.getResource` can be used to read. That is usually a better way to find fixtures because it avoids assuming the current working directory of the build.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org