zabetak commented on code in PR #3849:
URL: https://github.com/apache/calcite/pull/3849#discussion_r1679197453
##########
core/src/main/java/org/apache/calcite/rel/core/Filter.java:
##########
@@ -85,6 +86,8 @@ protected Filter(
RexNode condition) {
super(cluster, traits, child);
this.condition = requireNonNull(condition, "condition");
+ assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName()
Review 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.
##########
core/src/main/java/org/apache/calcite/rel/core/Join.java:
##########
@@ -100,6 +100,8 @@ protected Join(
JoinRelType joinType) {
super(cluster, traitSet, left, right);
this.condition = Objects.requireNonNull(condition, "condition");
+ assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName()
+ : "condition should be of BOOLEAN type, but was " +
condition.getType();
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
