[FLINK-34161][table] Migration of RewriteMinusAllRule to java #24143
Conversation
...ner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java
Show resolved
Hide resolved
| * Replaces logical {@link Minus} operator using a combination of union all, aggregate and table | ||
| * function. | ||
| * | ||
| * <p>Original Query : {@code SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2 } |
There was a problem hiding this comment.
It's not a big deal, but you might want to consider using a <pre> block to keep the readable formatting from the original scaladoc. Of course, you might have to escape > to > :/
There was a problem hiding this comment.
LGTM: that one issue is really only an internal consistency nitpick.
A quick question for the Immutable config -- is there a reason or a preference for using the withDescription(...) builders versus description(...) ? See https://github.com/apache/flink/pull/24141/files#diff-f3f8695553bb95921fa9346ecd8595e349d5ad5a182ff2681b6286a990f66727R111 for the other way to write these.
| List<Integer> fields = Util.range(minus.getRowType().getFieldCount()); | ||
|
|
||
| // 1. add vcol_marker to left rel node | ||
| RelBuilder leftBuilder = call.builder().push(left); |
There was a problem hiding this comment.
You've slightly changed the variable assignment here from the scala version (the left builder originally didn't include the .push(left)). This is fine since the push method returns the same instance, but it should probably be consistent below with the right side.
There was a problem hiding this comment.
This is actually a good catch, thanks, fixed
the main benefit is that with use of |
What is the purpose of the change
The PR migrates
RewriteMinusAllRuleto javait doesn't touch
RewriteMinusAllRuleTestto be sure that java version continues passing itVerifying this change
This change is already covered by existing tests
Does this pull request potentially affect one of the following parts:
@Public(Evolving): ( no)Documentation