[CALCITE-6948] Implement MinusToAntiJoinRule#4298
Conversation
mihaibudiu
added
the
LGTM-will-merge-soon
|
@mihaibudiu Hi, this pr has a minor change, please have a look thanks! |
| relBuilder.isNotDistinctFrom( | ||
| rexBuilder.makeInputRef(leftFieldType, i), | ||
| rexBuilder.makeInputRef(rightFieldType, i + fieldCount))); | ||
| if (!leftFieldType.isNullable() && !rightFieldType.isNullable()) { |
There was a problem hiding this comment.
I wonder whether this is enough.
You can join on ROW types, and in this case I wonder whether the check should be done on each field.
There was a problem hiding this comment.
I suddenly thought of a question. We need to judge whether it is nullable more precisely, so as to convert is not distinct from to =?
There was a problem hiding this comment.
it will work for scalar types as it is; you could check that
There was a problem hiding this comment.
Will it have any impact if we use is not distinct from uniformly and do not make more complex judgments (return to the logic of the previous version)? Maybe Relbuilder or other rule can reduce is not distinct from.
There was a problem hiding this comment.
the previous is correct, but this one can be slightly more efficient.
However, perhaps this kind of rewrite should be in the scope of a different rule, which could perform it everywhere?
There was a problem hiding this comment.
Personally I think it's cleaner if each optimization is done by a separate transformation rather than each transformation trying to perform many small optimizations by itself.
There was a problem hiding this comment.
I agree with you, I will convert the new commit soon.
|
|
If not one has comments I plan to merge this soon. |
|
|
||
| /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6948">[CALCITE-6948] | ||
| * Implement IntersectToSemiJoinRule</a>. */ | ||
| @Test void testMinusToAntiJoinRule() { |
There was a problem hiding this comment.
maybe you will be able to write a few quidem tests too after we merge #4301
There was a problem hiding this comment.
Yes, I am submitting #4301 to the draft branch now and would like everyone to see if this approach is acceptable. Then I will adjust the implementation method according to everyone's suggestions. The cases written in JdbcTest can all be completed using quidem tests.
2 participants
Please see: CALCITE-6948