You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2024/11/11 11:51:31 UTC
[PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Fokko opened a new pull request, #11514:
URL: https://github.com/apache/iceberg/pull/11514
Okay, I've added this to the spec a while ago:
https://github.com/apache/iceberg/pull/7445
But I think this was a mistake, and we should not expose this to the public APIs, as it is much better to track this internally.
I noticed this while reviewing https://github.com/apache/iceberg-rust/pull/587
Removing this as part of the APIs in Java, and the Open-API update makes it much more resilient, and don't require the clients to compute this value. For example. when there are two conflicting schema changes, the last-column-id must be recomputed correctly when doing the retry operation.
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1850550600
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -462,6 +465,8 @@ private static MetadataUpdate readAddSchema(JsonNode node) {
Schema schema = SchemaParser.fromJson(schemaNode);
int lastColumnId;
if (node.has(LAST_COLUMN_ID)) {
+ LOG.warn(
+ "Field last-column-id of MetadataUpdate.AddSchema is since 1.8.0, will be removed 1.9.0 or 2.0.0");
Review Comment:
```suggestion
"Field last-column-id of MetadataUpdate.AddSchema is deprecated since Iceberg 1.8.0, will be removed in 1.9.0 or 2.0.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
Re: [PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1853643848
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -462,6 +465,8 @@ private static MetadataUpdate readAddSchema(JsonNode node) {
Schema schema = SchemaParser.fromJson(schemaNode);
int lastColumnId;
if (node.has(LAST_COLUMN_ID)) {
+ LOG.warn(
+ "Field last-column-id of MetadataUpdate.AddSchema is deprecated since Iceberg 1.8.0, will be removed in 1.9.0 or 2.0.0");
Review Comment:
That works for me 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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1852719762
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1081,8 +1094,18 @@ public Builder setCurrentSchema(int schemaId) {
return this;
}
+ public Builder addSchema(Schema schema) {
+ addSchemaInternal(schema, schema.highestFieldId());
Review Comment:
I think this needs to be `Math.max(lastColumnId, schema.highestFieldId())` because the new schema could drop a column.
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #11514:
URL: https://github.com/apache/iceberg/pull/11514
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1853651877
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2692,7 +2692,15 @@ components:
$ref: '#/components/schemas/Schema'
last-column-id:
type: integer
- description: The highest assigned column ID for the table. This is used to ensure columns are always assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+ deprecated: true
+ description:
+ This optional field is **DEPRECATED for REMOVAL** since it more safe to handle this internally,
+ and shouldn't be exposed to the clients.
+
+ The highest assigned column ID for the table. This is used to ensure columns are always
+ assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+
+ Deprecated since Iceberg (Java) 1.8.0. The field will be removed from this spec in Iceberg (Java) 2.0.
Review Comment:
I took this approach from the deprecation of `getTokens`, but I agree. I've removed it for now.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2692,7 +2692,15 @@ components:
$ref: '#/components/schemas/Schema'
last-column-id:
type: integer
- description: The highest assigned column ID for the table. This is used to ensure columns are always assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+ deprecated: true
+ description:
+ This optional field is **DEPRECATED for REMOVAL** since it more safe to handle this internally,
+ and shouldn't be exposed to the clients.
+
+ The highest assigned column ID for the table. This is used to ensure columns are always
+ assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+
+ Deprecated since Iceberg (Java) 1.8.0. The field will be removed from this spec in Iceberg (Java) 2.0.
Review Comment:
I took this approach from the deprecation of `getToken`, but I agree. I've removed it 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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1843296963
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2692,7 +2692,14 @@ components:
$ref: '#/components/schemas/Schema'
last-column-id:
type: integer
- description: The highest assigned column ID for the table. This is used to ensure columns are always assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+ description:
Review Comment:
Nice one, 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
Re: [PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1850551275
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -562,10 +562,23 @@ public TableMetadata withUUID() {
return new Builder(this).assignUUID().build();
}
+ /**
+ * Updates the schema
+ *
+ * @deprecated since 1.8.0, will be removed 1.9.0 or 2.0.0, use updateSchema(schema).
Review Comment:
```suggestion
* @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0, use updateSchema(schema).
```
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1081,8 +1094,18 @@ public Builder setCurrentSchema(int schemaId) {
return this;
}
+ public Builder addSchema(Schema schema) {
+ addSchemaInternal(schema, schema.highestFieldId());
+ return this;
+ }
+
+ /**
+ * Add a new schema.
+ *
+ * @deprecated since 1.8.0, will be removed 1.9.0 or 2.0.0, use AddSchema(schema).
Review Comment:
same as above
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2477166647
@Fokko as part of the proposed deprecation, we should update all tests that use this (I found multiple 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
Re: [PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2490255141
@hussein-awala the Schema API is part of `iceberg-core` and thus allows things to be deprecated and then removed in the next minor release (see also https://iceberg.apache.org/contribute/#minor-version-deprecations-required). I'm guessing @Fokko mentioned 1.9.0 or 2.0.0 because we don't know yet whether there will be a 1.9.0 release or whether we go straight to 2.0.0 after 1.8.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
Re: [PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2470445349
> Do we have anyone or any implementations which benefit from having it there?
I have the same question, I can't think of why you need to set this externally.
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2468828703
I did not understand why this was there before. Do we have anyone or any implementations which benefit from having it there?
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1850688084
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1081,8 +1094,18 @@ public Builder setCurrentSchema(int schemaId) {
return this;
}
+ public Builder addSchema(Schema schema) {
+ addSchemaInternal(schema, schema.highestFieldId());
+ return this;
+ }
+
+ /**
+ * Add a new schema.
+ *
+ * @deprecated since 1.8.0, will be removed 1.9.0 or 2.0.0, use AddSchema(schema).
Review Comment:
```suggestion
* @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0, use AddSchema(schema).
```
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1852718855
##########
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java:
##########
@@ -462,6 +465,8 @@ private static MetadataUpdate readAddSchema(JsonNode node) {
Schema schema = SchemaParser.fromJson(schemaNode);
int lastColumnId;
if (node.has(LAST_COLUMN_ID)) {
+ LOG.warn(
+ "Field last-column-id of MetadataUpdate.AddSchema is deprecated since Iceberg 1.8.0, will be removed in 1.9.0 or 2.0.0");
Review Comment:
Do we need to warn on every update? This is going to fill service logs. We should just ignore this.
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1853645488
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1081,8 +1094,18 @@ public Builder setCurrentSchema(int schemaId) {
return this;
}
+ public Builder addSchema(Schema schema) {
+ addSchemaInternal(schema, schema.highestFieldId());
Review Comment:
Great catch! 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
Re: [PR] Core,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2483717731
@Fokko since this changes the REST Spec, you should probably hold a quick vote, but LGTM
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1842725865
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2692,7 +2692,14 @@ components:
$ref: '#/components/schemas/Schema'
last-column-id:
type: integer
- description: The highest assigned column ID for the table. This is used to ensure columns are always assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+ description:
Review Comment:
I think you can add the `deprecated: true` property for OpenAPI here 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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#issuecomment-2478125759
@danielcweeks You're right! I wanted to show that after updating the code, all the existing tests still pass.
I've updated the tests in a separate commit https://github.com/apache/iceberg/pull/11514/commits/f79cb11d6df697eb8e904698054a85eac50e18c6
--
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,Open-API: Don't expose the `last-column-id` [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11514:
URL: https://github.com/apache/iceberg/pull/11514#discussion_r1852721745
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2692,7 +2692,15 @@ components:
$ref: '#/components/schemas/Schema'
last-column-id:
type: integer
- description: The highest assigned column ID for the table. This is used to ensure columns are always assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+ deprecated: true
+ description:
+ This optional field is **DEPRECATED for REMOVAL** since it more safe to handle this internally,
+ and shouldn't be exposed to the clients.
+
+ The highest assigned column ID for the table. This is used to ensure columns are always
+ assigned an unused ID when evolving schemas. When omitted, it will be computed on the server side.
+
+ Deprecated since Iceberg (Java) 1.8.0. The field will be removed from this spec in Iceberg (Java) 2.0.
Review Comment:
This spec has nothing to do with Java releases, so I don't like including them here. A major Java release has meaning for Java consumers, but not to implementations of this spec.
--
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