nielspardon commented on code in PR #3703:
URL: https://github.com/apache/calcite/pull/3703#discussion_r1501122201
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey,
// duplicates, then add a Project.
final Set<AggregateCall> callSet = new HashSet<>();
final PairList<Integer, @Nullable String> projects = PairList.of();
- Util.range(groupSet.cardinality())
+ Util.range(groupSet2.cardinality())
Review Comment:
> Sure, but this means that #3700 is actually equivalent to this fix.
> So that PR probably fixes both issues as well.
Yes, functionally the other PR does fix the AssertionError and the
functional outcome is the same.
> Is there a more descriptive name that could be used instead of groupSet2?
Maybe that's the root problem: it is very hard to tell what groupSet2 means.
But if it was called e.g., replacementGroupSet, then it would become apparent
where it is misused.
Yes, I also think the problem was introduced because of the ambiguity of the
variables in the first place. A first thought I had was that maybe some
refactoring of this method into smaller methods like extracting the agg call
deduplication code into its own method could help to have a clearer separation
with less ambiguity.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
