Fix: Group transaction changes by table - #3360
Conversation
2380846 to
e0f05c0
Compare
|
nit: |
dimas-b
left a comment
There was a problem hiding this comment.
Nice fix. Thanks, @singhpk234 !
Just a couple of minor comments :)
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
8f1fe58
e0f05c0 to
8f1fe58
Compare
|
Apologies it took me a while to get back to it, was super swamped with internal stuff. Please have another pass when you all get some time, really apprecaite your feedbacks here ! |
|
@singhpk234 : thanks for pushing this forward 👍 Please fix fresh |
8f1fe58 to
90296e0
Compare
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks @singhpk234 !
| && !realmConfig() | ||
| .getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) { |
There was a problem hiding this comment.
This check allows the operation setLocation when ALLOW_NAMESPACE_LOCATION_OVERLAP is true. I think the behavior isn't correct. cc @dennishuo @collado-mike
However, it isn't a blocker for me as this PR doesn't the logic.
| Schema expectedSchema = updateSchema.apply(); | ||
| updateSchema.commit(); | ||
|
|
||
| transaction.updateProperties().set("prop-key", "prop-val").commit(); |
There was a problem hiding this comment.
Is behavior deterministic when we have two property updates like this?
- transaction.updateProperties().set("prop-key", "prop-val1").commit();
- transaction.updateProperties().set("prop-key", "prop-val2").commit();
If not, we may check with the Iceberg community to clarify the behavior. Not a blocker.
There was a problem hiding this comment.
within a transaction (taking iceberg's client side transcation as an example) it will be consistent as if prop-key value would be prop-val2 as we create a new metadata locally and keep on apply update on top of last updated metadata, is my understanding.
There was a problem hiding this comment.
Are updateRequests ordered? If not, the final result could be either val2 or val1.
There was a problem hiding this comment.
yes they are (atleast in java impl) I believe spec has an implicit assumption on this, we can iron the spec more bcz otherwise it will not be a true transaction if they are not ordered, is my take.
The serializer and deserializer both respect the order :
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/requests/CommitTransactionRequestParser.java#L42
an E2E tests for this
https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/rest/requests/TestCommitTransactionRequestParser.java#L162
plz let me know how feel about it
|
Thanks everyone for the review ! |
Problem
. Groups ALL changes by table- even if they appear randomly in the input like [A1, B1, A2, C1, A3] → groups to {A:
[A1, A2, A3], B: [B1], C: [C1]}
2. For each table, processes changes sequentially :
- Validate R1 against base metadata → Apply U1 → update currentMetadata
- Validate R2 against updated metadata → Apply U2 → update currentMetadata
- Validate R3 against updated metadata → Apply U3 → update currentMetadata
3. Single commit per table prevents duplicate entity IDs in pendingUpdates
This ensures:
related discussion: #3352 (comment)
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)