wombatu-kun commented on code in PR #16743:
URL: https://github.com/apache/iceberg/pull/16743#discussion_r3408267083
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -56,7 +56,8 @@ class SchemaUpdate implements UpdateSchema {
private final TableMetadata base;
private final Schema schema;
private final Map<Integer, Integer> idToParent;
- private final List<Integer> deletes = Lists.newArrayList();
+ // perf(auto-perf): Set for O(1) contains() — called 10x per schema apply
across all fields
Review Comment:
This comment is inaccurate. contains() is not called a fixed 10x per
apply(): the per-apply cost comes from ApplyChanges.field(), which the schema
visitor calls once per field, so the old List made it O(fields*deletes) - that
is exactly the O(fields^2) path this change fixes. The ~10 other
deletes.contains() sites live in the builder methods
(addColumn/renameColumn/updateColumn/...), each invoked once per API call, not
per apply. The `perf(auto-perf):` prefix and the non-ASCII em-dash are also not
used elsewhere in this file. Suggest dropping the comment (the rationale
already lives in the commit message and PR description) or replacing it with an
accurate one-liner.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
