You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rambleraptor (via GitHub)" <gi...@apache.org> on 2026/05/26 21:23:45 UTC
[PR] v4 - table metadata location should be optional [iceberg]
rambleraptor opened a new pull request, #16572:
URL: https://github.com/apache/iceberg/pull/16572
As part of the new relative paths spec, the field `location` on v4 table metadata has been made optional.
This PR ensures that location is optional on v4, but still required on v1-v3.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313178771
##########
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:
That sounds great. I broke this up into two different Preconditions to try to make it more readable (boolean conditions aren't always the easiest to read)
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #16572:
URL: https://github.com/apache/iceberg/pull/16572
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3321133094
##########
core/src/test/java/org/apache/iceberg/TestTables.java:
##########
@@ -133,7 +133,8 @@ private static TestTable createTable(
}
TableMetadata metadata =
- newTableMetadata(schema, spec, sortOrder, temp.toString(), properties, formatVersion);
+ newTableMetadata(
+ schema, spec, sortOrder, temp.toURI().toString(), properties, formatVersion);
Review Comment:
I tried this out locally and couldn't get it to work. The tests still required URI.create().
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3336576855
##########
api/src/main/java/org/apache/iceberg/Files.java:
##########
@@ -40,6 +40,9 @@ public static OutputFile localOutput(File file) {
}
public static OutputFile localOutput(String file) {
+ if (file.startsWith("file:")) {
Review Comment:
~We should probably pull this up to a constant rather than inlining the scheme.~
I did some searching and this is looks like the only pace it appears, so I'm ok with this 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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313621951
##########
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:
If I'm understanding the spec correctly, we're looking for absolute paths (beginning with '/') or paths with schemes.
I've added support for both on the precondition. We have *a lot* of v4 tests that have locations without schemes (but are absolute paths).
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313992965
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java:
##########
@@ -164,7 +164,7 @@ public void roundTripSerdeV3andHigher(int formatVersion) {
+ " \"metadata\" : {\n"
+ " \"format-version\" : %s,\n"
+ " \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n"
- + " \"location\" : \"location\",\n"
+ + " \"location\" : \"/location\",\n"
Review Comment:
This shouldn't be allowed. Table locations must be absolute.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314284151
##########
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:
Yep, that's what I was looking at when trying to determine what an absolute path is.
I'm going to open a separate PR shortly with the absolute path changes. We have a lot of places in the codebase where we're prefixing paths with '/' instead of a scheme and it'll pollute this PR a lot.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313174185
##########
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:
I like that so much more. I really dislike regexes, but that seemed easier at the time.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314000356
##########
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:
Agree. The confusion probably came from spec that said `location` is now optional and must be absolute when present. The definition of "absolute" here is that it must contain a URI scheme.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314374899
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java:
##########
@@ -164,7 +164,7 @@ public void roundTripSerdeV3andHigher(int formatVersion) {
+ " \"metadata\" : {\n"
+ " \"format-version\" : %s,\n"
+ " \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n"
- + " \"location\" : \"location\",\n"
+ + " \"location\" : \"file:/location\",\n"
Review Comment:
Minor: `file:/location` is a valid location per RFC 3986, but it is a bit unusual. Consdier using `file://location`?
##########
core/src/test/java/org/apache/iceberg/TestTables.java:
##########
@@ -133,7 +133,8 @@ private static TestTable createTable(
}
TableMetadata metadata =
- newTableMetadata(schema, spec, sortOrder, temp.toString(), properties, formatVersion);
+ newTableMetadata(
+ schema, spec, sortOrder, temp.toURI().toString(), properties, formatVersion);
Review Comment:
I think the change to use `toURI` in this class is necessitating `URI.create(...)` in downstream tests. This is because the Java URI class produces strings like `file:/location`, which `File` doesn't recognize. One way to avoid this is to build the scheme-prefixed path manually (e.g. `"file://" + temp.getAbsolutePath()`) in this class.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314011149
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -312,6 +313,19 @@ public String toString() {
SUPPORTED_TABLE_FORMAT_VERSION);
Preconditions.checkArgument(
formatVersion == 1 || uuid != null, "UUID is required in format v%s", formatVersion);
+ boolean locationOptional = formatVersion >= MIN_FORMAT_VERSION_OPTIONAL_LOCATION;
+ Preconditions.checkArgument(
+ locationOptional || location != null,
+ "Table location is required in format v%s",
+ formatVersion);
+ Preconditions.checkArgument(
+ !locationOptional
+ || location == null
+ || LocationUtil.hasScheme(location)
+ || location.startsWith("/"),
Review Comment:
```suggestion
|| LocationUtil.hasScheme(location),
```
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314284151
##########
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:
Yep, that's what I was looking at when trying to determine what an absolute path is. Made that change. There's a couple assorted test fixes as well to help make the new preconditions pass.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "kamcheungting-db (via GitHub)" <gi...@apache.org>.
kamcheungting-db commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3312817340
##########
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:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3336576855
##########
api/src/main/java/org/apache/iceberg/Files.java:
##########
@@ -40,6 +40,9 @@ public static OutputFile localOutput(File file) {
}
public static OutputFile localOutput(String file) {
+ if (file.startsWith("file:")) {
Review Comment:
We should probably pull this up to a constant rather than inlining the scheme.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#issuecomment-4557761330
Thanks a lot for the review! I've gone ahead and made the changes.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#issuecomment-4550379491
@anoopj this relates to the work you did around the relatives paths utilities. If you could take a look, I’d appreciate it. Thanks!
@danielcweeks @talatuyarer as the relative paths gurus as well
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313979286
##########
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:
A location should start with a URI scheme, not a `/`. For fields in metadata files, if a path starts with `/` it was likely introduced out of spec (even v3 and prior required a fully-qualified or URI with scheme value). We shouldn't allow for `/` prefixed paths in table metadata.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3313992965
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponseParser.java:
##########
@@ -164,7 +164,7 @@ public void roundTripSerdeV3andHigher(int formatVersion) {
+ " \"metadata\" : {\n"
+ " \"format-version\" : %s,\n"
+ " \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n"
- + " \"location\" : \"location\",\n"
+ + " \"location\" : \"/location\",\n"
Review Comment:
This shouldn't be allowed. Locations must be absolute.
--
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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3314284151
##########
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:
Yep, that's what I was looking at when trying to determine what an absolute path is.
I made some additional changes in the PR to handle Flink + Spark. Let me know if you want to break those out 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
Re: [PR] Core: v4 table metadata location should be optional [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #16572:
URL: https://github.com/apache/iceberg/pull/16572#discussion_r3326362157
##########
api/src/main/java/org/apache/iceberg/Files.java:
##########
@@ -40,6 +40,9 @@ public static OutputFile localOutput(File file) {
}
public static OutputFile localOutput(String file) {
+ if (file.startsWith("file:")) {
Review Comment:
We've done the same change over on InputFile before 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