linorosa commented on code in PR #4135:
URL: https://github.com/apache/calcite/pull/4135#discussion_r1912070021
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -1752,27 +1752,33 @@ private static void splitJoinCondition(
* {@code IS NOT DISTINCT FROM} function call.
*
* <p>For example: {@code t1.key IS NOT DISTINCT FROM t2.key}
- * can rewritten in expanded form as
+ * can be rewritten in expanded form as
* {@code t1.key = t2.key OR (t1.key IS NULL AND t2.key IS NULL)}.
*
- * @param call Function expression to try collapsing
+ * @param rexCall Function expression to try collapsing
* @param rexBuilder {@link RexBuilder} instance to create new {@link
RexCall} instances.
* @return If the given function is an expanded IS NOT DISTINCT FROM
function call,
- * return a IS NOT DISTINCT FROM function call. Otherwise return the
input
+ * return a IS NOT DISTINCT FROM function call. Otherwise, return
the input
* function call as it is.
*/
- public static RexCall collapseExpandedIsNotDistinctFromExpr(final RexCall
call,
+ public static RexCall collapseExpandedIsNotDistinctFromExpr(final RexCall
rexCall,
final RexBuilder rexBuilder) {
- switch (call.getKind()) {
- case OR:
- return doCollapseExpandedIsNotDistinctFromOrExpr(call, rexBuilder);
-
- case CASE:
- return doCollapseExpandedIsNotDistinctFromCaseExpr(call, rexBuilder);
-
- default:
- return call;
- }
+ final RexShuttle shuttle = new RexShuttle() {
+ @Override public RexNode visitCall(RexCall call) {
+ switch (call.getKind()) {
+ case OR:
+ return doCollapseExpandedIsNotDistinctFromOrExpr(call, rexBuilder);
+ case CASE:
+ return doCollapseExpandedIsNotDistinctFromCaseExpr(call, rexBuilder);
+ case NOT:
+ case AND:
+ return super.visitCall(call);
+ default:
+ return call;
Review Comment:
Yeah @asolimando my first instinct was to recursively collapse any `RexCall`
- and you're right I didn't recursively visit `OR`/`CASE` to collapse them. I
didn't want to rock the boat too much.
That said, I'm open to doing "the right thing" and rewrite this to collapse
anything recursively (i.e. both moving `return super.visitCall(call);` to the
`default` case and also taking care of collapsing `OR`/`CASE` operands.
The thing is, is it realistic to condition this change to adding tests for
everything under `SqlStdOperatorTable.*`? Or do you think you'd be comfortable
with testing some subset of them?
--
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
