julianhyde commented on a change in pull request #2253:
URL: https://github.com/apache/calcite/pull/2253#discussion_r545311273
##########
File path:
core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##########
@@ -366,14 +368,18 @@ public JoinReduceExpressionsRule(Class<? extends Join>
joinClass,
@Override public void onMatch(RelOptRuleCall call) {
final Join join = call.rel(0);
- final List<RexNode> expList = Lists.newArrayList(join.getCondition());
+ final RexBuilder rexBuilder = join.getCluster().getRexBuilder();
+ final RexSimplify rexSimplify =
+ new RexSimplify(rexBuilder, RelOptPredicateList.EMPTY, EXECUTOR);
+ final RexNode condition =
rexSimplify.eliminateCommonExprInCondition(join.getCondition());
+
+ final List<RexNode> expList = Lists.newArrayList(condition);
Review comment:
The current PR is very unclear.
It doesn't state which component is being improved. I guess it's
JoinReduceExpressionsRule?
It adds a method to RexSimplify which is orthogonal to the other purposes of
RexSimplify. That's like adding a method to StringBuilder to also tell you
whether it's going to rain today.
There are changes to parameter types of public methods. Let's make sure that
is not breaking.
Changing to use `HashSet` instead of `List` will cause optimization to
become nondeterministic. One option is to switch to `LinkedHashSet`. But it is
rather expensive. You should seriously consider continuing to use lists but
just ensure that they are unique when you build them. This pattern works well:
```
final Set<String> set = new HashSet<>();
final List<String> list = new ArrayList<>();
for (s in source()) {
if (set.add(s)) {
list.add(s);
}
}
return list;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
