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 2021/11/21 22:32:09 UTC
[GitHub] [iceberg] rdblue opened a new pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
rdblue opened a new pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592
This adds methods to parse `TableMetadata` from JSON strings, not just from `InputFile`. This is in support of the REST catalog.
--
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 change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753881595
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
+ return fromJson(io, null, json);
Review comment:
This is also validated by the tests, which call this new method quite a bit.
--
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 change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753870339
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
+ return fromJson(io, null, json);
Review comment:
No, that method expects `JsonNode`, not a String. This is calling `fromJson(FileIO, String, String)`.
--
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 change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753870270
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
Review comment:
You can pass `null` for `FileIO` in those cases. It isn't used unless you call methods on `Snapshot` that need to open files, like `addedFiles`. To fix that, I think we want to gracefully handle missing `FileIO` in `Snapshot`, but that's 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
[GitHub] [iceberg] danielcweeks commented on a change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753865383
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
+ return fromJson(io, null, json);
Review comment:
doesn't this `null` cause NPE on line 288?
--
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] danielcweeks commented on a change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r754640891
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
+ return fromJson(io, null, json);
Review comment:
ok, IDE must have been confused because it was traversing differently (I was also getting ambiguous method references).
--
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 change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753870339
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
+ return fromJson(io, null, json);
Review comment:
No, that method expects `JsonNode`, not a String. This is calling the new `fromJson(FileIO, String, String)` below.
--
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] danielcweeks commented on a change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753864659
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
Review comment:
For serializing/deserializing with REST catalog, it would be nice to have an option to parse the metadata and then later bind in the FileIO.
--
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 change in pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592#discussion_r753871444
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -253,8 +254,42 @@ public static TableMetadata read(FileIO io, InputFile file) {
}
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ /**
+ * Read TableMetadata from a JSON string.
+ * <p>
+ * The TableMetadata's metadata file location will be unset.
+ *
+ * @param io a FileIO used by {@link Snapshot} instances
+ * @param json a JSON string of table metadata
+ * @return a TableMetadata object
+ */
+ public static TableMetadata fromJson(FileIO io, String json) {
Review comment:
I went to add the Snapshot changes and they're actually smaller than I thought so I'll add them in this PR. I still think it's a good idea to require `FileIO` in the method because we want most people to always pass one.
--
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] danielcweeks merged pull request #3592: Core: Add methods to parse TableMetadata from JSON strings
Posted by GitBox <gi...@apache.org>.
danielcweeks merged pull request #3592:
URL: https://github.com/apache/iceberg/pull/3592
--
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