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/07/14 07:59:12 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #5275: Write metadata-location instead of metadataLocation
Fokko opened a new pull request, #5275:
URL: https://github.com/apache/iceberg/pull/5275
While talking to the REST API I noticed that the API returns `metadataLocation`, instead of `metadata-location` as defined in the Open API Spec: https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L1486
--
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 #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r921347956
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
Review Comment:
Do we need a null check here? I believe this throws if `keyValue.getValue()` is null.
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
+ }
+ generator.writeEndObject();
+
+ generator.writeEndObject();
+ }
+
+ public static LoadTableResponse fromJson(JsonNode node) {
+ Preconditions.checkArgument(node.isObject(),
+ "Cannot parse table response from a non-object: %s", node);
+
+ String metadataLocation = null;
+ JsonNode metadataLocationNode = node.get(METADATA_LOCATION);
+ if(metadataLocationNode != null) {
+ metadataLocation = metadataLocationNode.asText();
+ }
Review Comment:
You can use `String metadataLocation = JsonUtil.getStringOrNull(METADATA_LOCATION, node)` for convenience here.
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -89,8 +144,15 @@ public static class Builder {
private Builder() {
}
+ public Builder withMetadataLocation(String metadataLocation) {
+ this.metadataLocation = metadataLocation;
+ return this;
+ }
Review Comment:
+1 to this. I find that sometimes, the `TableMetadata` that gets passed has `null` for `metadataLocation`, in which case we need to use the `TableMetadataBuilder` to add it which isn't very convenient. 👍
--
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] Fokko closed pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
Fokko closed pull request #5275: Write metadata-location instead of metadataLocation
URL: https://github.com/apache/iceberg/pull/5275
--
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 #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r933881451
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
Review Comment:
I don't think there should be any null values.
--
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 #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#issuecomment-1184679637
For testing this, if you override serialize in `TestLoadTableResponse` like [this example for TestOAuthTokenResponse](https://github.com/apache/iceberg/blob/6a06cb3bd24c101d52e7987f55039b6b9692831d/core/src/test/java/org/apache/iceberg/rest/responses/TestOAuthTokenResponse.java#L60-L63)
--
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] Fokko commented on pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#issuecomment-1215775866
@rdblue I missed your comment here. I agree that we don't need the custom encoder, and I'll close this one. The bug was on our impl that has been fixed a while ago 👍🏻
--
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 pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#issuecomment-1200283981
@Fokko, do you think we should move forward with this? As Kyle mentioned, if you use the included `RESTObjectMapper.mapper()` then the fields are sent and parsed correctly (as `metadata-location`). Did that solve the issue you ran into?
We were thinking that it wouldn't be necessary to add hand-coded parsers for each request/response class if we can ensure that the right mapper is always used. We do that in at least the client code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r922418866
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
Review Comment:
Technically speaking, when using Jackson's `JsonGenerator`, if the map _does_ allow for `null` values, I think something like the following is needed:
```java
if (keyValue.getValue() != null) {
generate.writeStringField(keyValue.getKey(), keyValue.getValue());
} else {
generate.writeNullField(keyValue.getKey());
}
```
Usually we tend to convert input Maps to Guava's `ImmutableMap` using `ImmutableMap.copyOf()` like here in [TableMetadata](https://github.com/apache/iceberg/blob/64ed2a7e2b5f7acce921e6d2c40d97ef276c44e8/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1244-L1245). Guava's `ImmutableMap` does not allow for `null` values (and its `copyOf` method is a no-op if the map in question is already an `ImmutableMap` so it's ok to use regularly).
But `LoadTableResponse` actually does not [return an immutable map copy](https://github.com/apache/iceberg/blob/95f9adba722d192f79f26fdf94d90fc8d8e4bc50/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java#L67-L69), in case the person defining the REST catalogs backend has assigned some special meaning to `null` values in their configuration object.
--
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 #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r922418866
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
Review Comment:
Technically speaking, when using Jackson's `JsonGenerator`, if the map _does_ allow for `null` values, I think something like the following is needed:
```java
if (keyValue.getValue() != null) {
generator.writeStringField(keyValue.getKey(), keyValue.getValue());
} else {
generator.writeNullField(keyValue.getKey());
}
```
Usually we tend to convert input Maps to Guava's `ImmutableMap` using `ImmutableMap.copyOf()` like here in [TableMetadata](https://github.com/apache/iceberg/blob/64ed2a7e2b5f7acce921e6d2c40d97ef276c44e8/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1244-L1245). Guava's `ImmutableMap` does not allow for `null` values (and its `copyOf` method is a no-op if the map in question is already an `ImmutableMap` so it's ok to use regularly).
But `LoadTableResponse` actually does not [return an immutable map copy](https://github.com/apache/iceberg/blob/95f9adba722d192f79f26fdf94d90fc8d8e4bc50/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java#L67-L69), in case the person defining the REST catalogs backend has assigned some special meaning to `null` values in their configuration object.
--
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] Fokko commented on a diff in pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r922399128
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
+ }
+ generator.writeEndObject();
+
+ generator.writeEndObject();
+ }
+
+ public static LoadTableResponse fromJson(JsonNode node) {
+ Preconditions.checkArgument(node.isObject(),
+ "Cannot parse table response from a non-object: %s", node);
+
+ String metadataLocation = null;
+ JsonNode metadataLocationNode = node.get(METADATA_LOCATION);
+ if(metadataLocationNode != null) {
+ metadataLocation = metadataLocationNode.asText();
+ }
Review Comment:
Ooh, that's nice. Thanks!
--
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] Fokko commented on a diff in pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5275:
URL: https://github.com/apache/iceberg/pull/5275#discussion_r922395078
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -37,6 +42,56 @@
*/
public class LoadTableResponse implements RESTResponse {
+ private static final String METADATA_LOCATION = "metadata-location";
+ private static final String METADATA = "metadata";
+ private static final String CONFIG = "config";
+
+ public static void toJson(LoadTableResponse loadTable, JsonGenerator generator) throws IOException {
+ generator.writeStartObject();
+
+ generator.writeStringField(METADATA_LOCATION, loadTable.metadataLocation);
+
+ generator.writeFieldName(METADATA);
+ TableMetadataParser.toJson(loadTable.metadata, generator);
+
+ generator.writeObjectFieldStart(CONFIG);
+ for (Map.Entry<String, String> keyValue : loadTable.config.entrySet()) {
+ generator.writeStringField(keyValue.getKey(), keyValue.getValue());
Review Comment:
I copied this from the `TableMetadataParser` 🙈 Browsing through the code, I don't see any null checks.
--
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] Fokko closed pull request #5275: Write metadata-location instead of metadataLocation
Posted by GitBox <gi...@apache.org>.
Fokko closed pull request #5275: Write metadata-location instead of metadataLocation
URL: https://github.com/apache/iceberg/pull/5275
--
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