Core,Open-API: Don't expose the last-column-id
#11514
Conversation
|
I did not understand why this was there before. 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. |
Fokko
force-pushed
the
fd-remove-last-column-id
branch
from
c8fcb68 to
6870e60
Compare
|
@Fokko as part of the proposed deprecation, we should update all tests that use this (I found multiple references). |
Okay, I've added this to the spec a while ago: apache#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 apache/iceberg-rust#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.
Fokko
force-pushed
the
fd-remove-last-column-id
branch
from
0d935f5 to
f79cb11
Compare
|
@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 f79cb11 |
There was a problem hiding this comment.
LGTM, I checked references to last-column-id and last_column_id and didn't find anything that would break due to this change.
https://grep.app/search?current=4&q=last-column-id&filter[lang][0]=Python&filter[lang][1]=Rust&filter[lang][2]=Java&filter[lang][3]=Go&filter[lang][4]=C%2B%2B
https://grep.app/search?q=last_column_id
|
@Fokko since this changes the REST Spec, you should probably hold a quick vote, but LGTM |
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Eduard Tudenhoefner <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>
There was a problem hiding this comment.
deprecated since 1.8.0, will be removed 1.9.0 or 2.0.0, use AddSchema(schema).
I wonder if this interface is part of Iceberg public API, because from v1.0.0 release note:
From 1.0.0 forward, the project will follow semver in the public API module, iceberg-api.
we cannot introduce breaking changes to the public API in a patch or minor version, and we need to wait for the next major version to introduce it, in our case 2.0.0, and if it's considered as a part of the public API, then you need to replace will be removed 1.9.0 or 2.0.0 by will be removed in 2.0.0
|
@hussein-awala the Schema API is part of |
core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java
Outdated
Show resolved
Hide resolved
…nto fd-remove-last-column-id
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
* Core,Open-API: Don't expose the `last-column-id` Okay, I've added this to the spec a while ago: apache#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 apache/iceberg-rust#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. * Update the tests as well * Add `deprecation` flag * Wording Co-authored-by: Eduard Tudenhoefner <[email protected]> * Wording Co-authored-by: Eduard Tudenhoefner <[email protected]> * Wording * Thanks Ryan! * Remove `LOG` --------- Co-authored-by: Eduard Tudenhoefner <[email protected]>
This should not be part of the public API: apache/iceberg#11514 This PR depends on a later version of the REST catalog for the integration tests.
Okay, I've added this to the spec a while ago:
#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 apache/iceberg-rust#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.