You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "snazy (via GitHub)" <gi...@apache.org> on 2023/03/07 11:35:55 UTC
[GitHub] [iceberg] snazy opened a new pull request, #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
snazy opened a new pull request, #7032:
URL: https://github.com/apache/iceberg/pull/7032
In the WIP for the Nessie Iceberg REST catalog server, there are a few places that currently require us to first serialize a `TableMetadata` to a `String` just to parse it right back into a `TableMetadata`, but this time with the right metadata-location.
First occurence is when we write the table metadata, where we have to first store the table metadata, get the metadata location, and then return the table metadata with the metadata location (and updates cleared). Second occurence is when we return a table metadata with "transient properties", which are used to pass the "expected Nessie commit" around.
This change makes the two `TableMetadataParser.fromJson()` taking a `JsonNode` public
--
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 merged pull request #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #7032:
URL: https://github.com/apache/iceberg/pull/7032
--
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] snazy commented on pull request #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #7032:
URL: https://github.com/apache/iceberg/pull/7032#issuecomment-1613170737
Yes, would be nice (for us) to have to avoid some unnecessary re-serialization round-trips.
--
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 #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7032:
URL: https://github.com/apache/iceberg/pull/7032#discussion_r1268289514
##########
core/src/main/java/org/apache/iceberg/TableMetadataParser.java:
##########
@@ -299,7 +299,7 @@ public static TableMetadata fromJson(String metadataLocation, String json) {
return JsonUtil.parse(json, node -> TableMetadataParser.fromJson(metadataLocation, node));
}
- static TableMetadata fromJson(InputFile file, JsonNode node) {
+ public static TableMetadata fromJson(InputFile file, JsonNode node) {
Review Comment:
@Fokko, @snazy, I think we need to revert this. We cannot leak Jackson classes publicly through an Iceberg API, even in core.
--
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 #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7032:
URL: https://github.com/apache/iceberg/pull/7032#discussion_r1269984651
##########
core/src/main/java/org/apache/iceberg/TableMetadataParser.java:
##########
@@ -299,7 +299,7 @@ public static TableMetadata fromJson(String metadataLocation, String json) {
return JsonUtil.parse(json, node -> TableMetadataParser.fromJson(metadataLocation, node));
}
- static TableMetadata fromJson(InputFile file, JsonNode node) {
+ public static TableMetadata fromJson(InputFile file, JsonNode node) {
Review Comment:
I talked to Fokko about this and I think it should be okay. I didn't realize we were already doing this elsewhere. Sorry for the confusion!
--
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 #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #7032:
URL: https://github.com/apache/iceberg/pull/7032#issuecomment-1613185392
Thanks @snazy for opening up this PR, and @nastra for the review
--
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] nastra commented on pull request #7032: Make `TableMetadataParser.fromJson` taking `JsonNode` public
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7032:
URL: https://github.com/apache/iceberg/pull/7032#issuecomment-1613165947
@snazy is this still required?
--
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