adnanhemani commented on code in PR #3456:
URL: https://github.com/apache/polaris/pull/3456#discussion_r2714866291
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java:
##########
@@ -174,6 +175,8 @@ private EventAttributes() {}
// Transaction attributes
public static final AttributeKey<CommitTransactionRequest>
COMMIT_TRANSACTION_REQUEST =
new AttributeKey<>("commit_transaction_request",
CommitTransactionRequest.class);
+ public static final AttributeKey<List<LoadTableResponse>>
LOAD_TABLE_RESPONSES =
Review Comment:
Changed.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -783,7 +787,14 @@ public Response commitTransaction(
new AttributeMap()
.put(EventAttributes.CATALOG_NAME, catalogName)
.put(EventAttributes.COMMIT_TRANSACTION_REQUEST,
commitTransactionRequest)));
- for (UpdateTableRequest req : commitTransactionRequest.tableChanges()) {
+ List<LoadTableResponse> loadTableResponses =
+ attributeMap.getRequired(EventAttributes.LOAD_TABLE_RESPONSES);
+ for (int i = 0; i < commitTransactionRequest.tableChanges().size(); i++) {
+ UpdateTableRequest req = commitTransactionRequest.tableChanges().get(i);
+ LoadTableResponse loadTableResponse =
+ loadTableResponses != null && i < loadTableResponses.size()
+ ? loadTableResponses.get(i)
+ : null;
Review Comment:
> My concern is that it is an implicit assumption if one ever changes, the
other place will also have to change, and I don't think unit tests will catch
this case.
For this implicit assumption to change, `tableChanges` would have to become
an unordered collection. Given that it's coming from the Iceberg REST Spec, I
don't think that's very likely at all. Happy to hear if you have a
non-intrusive way to make a change to future-proof this.
> The point I was trying to make above is that from my POV it is odd to have
both LOAD_TABLE_RESPONSE and LOAD_TABLE_RESPONSES (plural) in EventAttributeMap.
I see your point now. I'm not sure that I agree or disagree with this
recommendation 100% - but I will implement for now and we can revisit if it
ends up being an issue in the future.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
