Merged
Conversation
Contributor
|
just an FYI that there's also #6701 to address the same |
nastra
approved these changes
Apr 27, 2023
singhpk234
approved these changes
Apr 27, 2023
rdblue
reviewed
Apr 27, 2023
dramaticlly
approved these changes
Apr 27, 2023
nastra
reviewed
May 1, 2023
Contributor
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
change itself LGTM, but it would be good to add a test to TestMetadataUpdateParser where we don't specify last-column-id:
@Test
public void testAddSchemaFromJsonWithoutLastColumnId() {
String action = MetadataUpdateParser.ADD_SCHEMA;
Schema schema = ID_DATA_SCHEMA;
int lastColumnId = schema.highestFieldId();
String json =
String.format("{\"action\":\"add-schema\",\"schema\":%s}", SchemaParser.toJson(schema));
MetadataUpdate actualUpdate = new MetadataUpdate.AddSchema(schema, lastColumnId);
assertEquals(action, actualUpdate, MetadataUpdateParser.fromJson(json));
}
Contributor
Author
|
@nastra Excellent idea, added a test! 👍🏻 |
Fokko
force-pushed
the
fd-add-missing-last-column-id
branch
from
07e8839 to
10a1af9
Compare
Fokko
force-pushed
the
fd-add-missing-last-column-id
branch
from
10a1af9 to
443b797
Compare
rdblue
approved these changes
Jun 5, 2023
Contributor
Author
|
Thanks @nastra, @dramaticlly, @singhpk234 and @rdblue for the review! 🙌🏻 |
nastra
pushed a commit
to nastra/iceberg
that referenced
this pull request
Aug 15, 2023
* Spec: Add missing last-column-id to open-api spec * Add description
Fokko
added a commit
to Fokko/iceberg
that referenced
this pull request
Nov 11, 2024
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
Fokko
added a commit
to Fokko/iceberg
that referenced
this pull request
Nov 11, 2024
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
added a commit
to Fokko/iceberg
that referenced
this pull request
Nov 15, 2024
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
added a commit
that referenced
this pull request
Nov 25, 2024
* Core,Open-API: Don't expose the `last-column-id` 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. * 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]>
zachdisc
pushed a commit
to zachdisc/iceberg
that referenced
this pull request
Dec 23, 2024
* 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed that this was required:
Java code:
iceberg/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java
Lines 397 to 402 in 882459d