zabetak commented on code in PR #4089:
URL: https://github.com/apache/calcite/pull/4089#discussion_r1891437875
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdUniqueKeys.java:
##########
@@ -184,10 +213,14 @@ private static Set<ImmutableBitSet>
getProjectUniqueKeys(SingleRel rel, RelMetad
// the resulting unique keys would be {{0},{4}}, {{1},{4}}
Iterable<List<Integer>> product = Linq4j.product(Util.transform(colMask,
mapInToOutPos::get));
-
- resultBuilder.addAll(Util.transform(product, ImmutableBitSet::of));
+ for (List<Integer> passKey : product) {
+ if (resultBuilder.size() == limit) {
Review Comment:
We can't simply move the check after adding to the set. Consider for
instance the case where limit is zero. If we first add an element and then
check the limit the comparison will be wrong (`(size) 1 == (limit) 0`).
We could avoid the extra loop by duplicating the limit check but this makes
the code slightly less readable; I opted to sacrifice a bit of performance for
readability.
Since this is a minor concern, I will merge this PR as is. If you feel that
refining the limit check is necessary please let me know and I will open a
follow-up ticket.
--
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]
