[CALCITE-6448] FilterReduceExpressionsRule returns empty RelNode for R… #3849
Conversation
soumyakanti3578
force-pushed
the
CALCITE-6448
branch
from
811dfbb
to
91e6c13
Compare
soumyakanti3578
changed the title
| @@ -85,6 +86,8 @@ protected Filter( | |||
| RexNode condition) { | |||
| super(cluster, traits, child); | |||
| this.condition = requireNonNull(condition, "condition"); | |||
| assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() | |||
There was a problem hiding this comment.
I am not sure this is always true; for example, before validation where casts are introduced this may not hold.
Also, Calcite does not always represent SQL implicit casts explicitly, so the cast to Boolean which is implied here may not be in the program.
There was a problem hiding this comment.
The SQL level (SqlNode) and the algebraic level (Rel/RexNode) are handled differently. The validator as well as the implicit casts logic apply on the SQL level and not at the algebraic level. The algebra is explicit so the assertion for Boolean type makes sense to me.
Other than that it may be better to put the check inside the isValid method which is the canonical way of checking validity and allows also overriding in case some specialized implementation wants to perform the validity check differently.
| b.equals(b.field(2, 0, "DEPTNO"), b.literal(2)), | ||
| b.equals(b.field(2, 1, "DEPTNO"), | ||
| b.field(2, 0, "DEPTNO"))))) | ||
| b.and(b.equals(b.field(2, 0, "DEPTNO"), b.literal(1)), |
There was a problem hiding this comment.
why are you modifying this test?
There was a problem hiding this comment.
Keeping a NULL literal makes the type of the condition to be of type NULL. This PR is not ready for review, and I am just testing things as tests are failing on my local machine with unrelated errors. I will mark this PR as draft for now.
| @@ -145,76 +145,76 @@ public void testMatch() { | |||
| } | |||
|
|
|||
| @Test void testAdd() { | |||
| checkTranslation("b + 3", inTree("+($1, 3)")); | |||
| checkTranslation("(boolean)(b + 3)", inTree("+($1, 3)")); | |||
There was a problem hiding this comment.
what is the meaning of this expression?
| @@ -85,6 +86,8 @@ protected Filter( | |||
| RexNode condition) { | |||
| super(cluster, traits, child); | |||
| this.condition = requireNonNull(condition, "condition"); | |||
| assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() | |||
There was a problem hiding this comment.
The SQL level (SqlNode) and the algebraic level (Rel/RexNode) are handled differently. The validator as well as the implicit casts logic apply on the SQL level and not at the algebraic level. The algebra is explicit so the assertion for Boolean type makes sense to me.
Other than that it may be better to put the check inside the isValid method which is the canonical way of checking validity and allows also overriding in case some specialized implementation wants to perform the validity check differently.
| assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() | ||
| : "condition should be of BOOLEAN type, but was " + condition.getType(); |
There was a problem hiding this comment.
The check already exists inside the isValid method. What we may want to do here is include
assert isValid in the constructor as it is done in other RelNodes.
There was a problem hiding this comment.
For Join, I tried adding assert isValid in the constructor, but CheckerFramework throws an error:
> Task :core:compileJava
/home/runner/work/calcite/calcite/core/src/main/java/org/apache/calcite/rel/core/Join.java:107: error: [method.invocation.invalid] call to isValid(org.apache.calcite.util.Litmus,@org.checkerframework.checker.nullness.qual.Nullable org.apache.calcite.rel.RelNode.Context) not allowed on the given receiver.
assert isValid(Litmus.THROW, null);
^
found : @UnderInitialization(org.apache.calcite.rel.core.Join.class) @NonNull Join
required: @Initialized @NonNull Join
It looks like we can't call it from the constructor without changing some config? Or it may be simpler to assert that the condition type is boolean in the constructor - just for Join.
I encountered another issue with calling isValid from the constructor. During LogicalJoin instantiation, it calls super and tries to init Join and then it goes through isValid -> getRowType -> deriveRowType -> getSystemFieldList. But LogicalJoin#getSystemFieldList simply returns systemFieldList and its value is null because the object is still initializing and unfortunately super has to be the first call in a constructor (for now). Finally we get an error because of the requireNonNull check on systemFieldList in SqlValidatorUtil#deriveJoinRowType.
…exLiterals
Add a check in Join and Filter to require conditions to be of type BOOLEAN