anoopj commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3312215358
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -312,6 +313,10 @@ public String toString() {
SUPPORTED_TABLE_FORMAT_VERSION);
Preconditions.checkArgument(
formatVersion == 1 || uuid != null, "UUID is required in format v%s",
formatVersion);
+ Preconditions.checkArgument(
Review Comment:
V4 makes table location optional, but it should be absolute when present. We
should consider adding the absolute validation here. We can promote
`hasScheme()` that was added in https://github.com/apache/iceberg/pull/16174 to
be public so that it can be used here.
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -1133,6 +1133,64 @@ public void testParserVersionValidation() throws
Exception {
.hasMessageStartingWith("Cannot read unsupported version");
}
+ @ParameterizedTest
+ @ValueSource(ints = {1, 2, 3})
+ public void testLocationRequiredBeforeV4(int formatVersion) {
+ assertThatThrownBy(
+ () ->
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA,
+ PartitionSpec.unpartitioned(),
+ SortOrder.unsorted(),
+ null,
+ ImmutableMap.of(),
+ formatVersion))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Table location is required in format v%s", formatVersion);
+ }
+
+ @ParameterizedTest
+ @ValueSource(ints = {1, 2, 3})
+ public void testParserRequiresLocationBeforeV4(int formatVersion) {
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA,
+ PartitionSpec.unpartitioned(),
+ SortOrder.unsorted(),
+ TEST_LOCATION,
+ ImmutableMap.of(),
+ formatVersion);
+ // drop the location field to simulate a v1-v3 metadata file that omits
the required location
+ String withoutLocation =
+ TableMetadataParser.toJson(metadata)
+ .replaceFirst("\"location\"\\s*:\\s*\"[^\"]*\"\\s*,", "");
Review Comment:
Using regex is a bit fragile (e.g. if location ever becomes the last field
where it doesn't have a comma). Consider doing something like:
```
ObjectNode node = (ObjectNode)
JsonUtil.mapper().readTree(TableMetadataParser.toJson(metadata));
node.remove("location");
String withoutLocation = JsonUtil.mapper().writeValueAsString(node);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
