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/06/20 01:27:42 UTC
[GitHub] [iceberg] bryanck opened a new pull request, #5084: REST: Set table format version for create table transactions
bryanck opened a new pull request, #5084:
URL: https://github.com/apache/iceberg/pull/5084
This PR adds the table format version in the list of metadata updates when starting a create table transaction with the REST catalog client.
--
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 merged pull request #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5084:
URL: https://github.com/apache/iceberg/pull/5084
--
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 #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r902028547
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
+1. But I too see what you mean.
I could have sworn I left an almost identical comment when I reviewed this before. I can possibly help with adding a unit test for this sequence in `CatalogTests` if we’d like. Feel free to reach out on Slack too.
--
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] bryanck commented on a diff in pull request #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r901997846
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
I think the server-side default and the client-side default may not match? That is the issue I originally encountered, the Trino plugin had v2 as the default and set the initial sequence number to 1, but the server didn't receive a format version so treated it as v1, and failed because the sequence number was > 0.
--
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] bryanck commented on pull request #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
bryanck commented on PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#issuecomment-1160917682
I added a test for creating a V2 table via a create transaction, which fails without the change.
--
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 #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r902028547
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
+1.
I could have sworn I left an almost identical comment when I reviewed this before (the single comment I was referring to). I can possibly help with adding a unit test for this sequence in `CatalogTests` if we’d like. Feel free to reach out on Slack too.
I too see what you mean.
--
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 #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r901948667
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
I think this should be set only if the format version is not the default?
--
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 #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r902027505
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
Okay, I see what you mean.
--
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] bryanck commented on a diff in pull request #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r901997846
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
I think the server-side default and the client-side default may not match? The issue I originally encountered was the Trino plugin had v2 as the default and set the initial sequence number to 1, but the server didn't receive a format version so treated it as v1, and failed because the sequence number was > 0. If the client and server are on different Iceberg versions, there could potentially be a mismatch in the default.
--
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] bryanck commented on a diff in pull request #5084: REST: Set table format version for create table transactions
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #5084:
URL: https://github.com/apache/iceberg/pull/5084#discussion_r901997846
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Review Comment:
I think the server-side default and the client-side default may not match? That is the issue I originally encountered, the Trino plugin had v2 as the default and set the initial sequence number to 1, but the server didn't receive a format version so treated it as v1, and failed because the sequence number was > 0. If the client and server are on different Iceberg versions, there could potentially be a mismatch also.
--
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