HIVE-28910: Remove redundant IS NOT NULL predicates when expanding SEARCH #5795
Conversation
| case IS_NOT_NULL: | ||
| case CASE: | ||
| case COALESCE: | ||
| // Everything below must be transformed with the UNKNOWN handler |
There was a problem hiding this comment.
Can you elaborate on why you can't use the outermost handler here?
In the Jira ticket you seem towards the opposite direction, that is taking into account that in WHERE clauses filters obey to the unknownAsFalse semantics.
I haven't reviewed the Calcite upgrade which introduced some of the code you touch here, so maybe it should be clear from the context.
There was a problem hiding this comment.
There are some operators with special semantics that we need to retain the 3-valued logic even if we are inside the WHERE clause.
Example: COALESCE(SEARCH(...), true)
- If SEARCH -> true then COALESCE -> true
- If SEARCH -> false then COALESCE -> false
- If SEARCH -> null then COALESCE -> true
If we consider unknownAsFalse inside the COALESCE then we will incorrectly drop the IS NOT NULL predicate during the expansion and the COALESCE will fall into the third case returning true instead of false.
Basically, if the SEARCH is below/inside an operator that is sensitive to null values we cannot safely perform the simplification.
Note that RexSimplify also changes the default unknown semantics for certain operators (see simplifyIs2, simplifyCoalesce, etc.).
There was a problem hiding this comment.
I suspected something similar, thanks for the clarification.
Since this is a rather deep technical detail that most people not familiar with CBO will have problems with, I'd suggest to have a more explicit comment like "// These operator kinds require the UNKNOWN handler to correctly handle NULL values" or something similar.
I know it's still a draft PR but for the rest LGTM already, feel free to ping me for a review if needed.
There was a problem hiding this comment.
I added more comments and also made the approach a bit more defensive in the sense that if SEARCH happens to be under any operator other than AND/OR we don't perform the simplification.
Some of the expressions cannot be tested with end-to-end tests since some expressions are simplified even before hitting this code.
asf-ci-hive
added
tests pending
tests failed
and removed
tests unstable
tests pending
labels
The following error appears when running vector_case_when_2.q java.lang.AssertionError at org.apache.calcite.sql.fun.SqlCastFunction.inferReturnType(SqlCastFunction.java:98) at org.apache.calcite.rex.RexBuilder.deriveReturnType(RexBuilder.java:292) at org.apache.calcite.rex.RexBuilder.makeCall(RexBuilder.java:266) at org.apache.hadoop.hive.ql.optimizer.calcite.SearchTransformer$Shuttle.visitCall(SearchTransformer.java:151) at org.apache.hadoop.hive.ql.optimizer.calcite.SearchTransformer$Shuttle.visitCall(SearchTransformer.java:107) at org.apache.calcite.rex.RexCall.accept(RexCall.java:189) at org.apache.calcite.rex.RexShuttle.apply(RexShuttle.java:275) at org.apache.calcite.rex.RexShuttle.mutate(RexShuttle.java:243) at org.apache.calcite.rex.RexShuttle.apply(RexShuttle.java:263) at org.apache.calcite.rel.core.Project.accept(Project.java:188) at org.apache.hadoop.hive.ql.optimizer.calcite.rules.HiveRexShuttleTransformRule.onMatch(HiveRexShuttleTransformRule.java:37)
asf-ci-hive
added
tests pending
tests unstable
tests failed
and removed
tests failed
tests pending
tests unstable
labels
|
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveInBetweenExpandRule.java
Show resolved
Hide resolved
|
I added two commits to sync with master and resolve conflicts. If it comes green I will merge the PR tomorrow. |
|
|
Thanks for the reviews @kasakrisz and @asolimando ! |
Does this PR introduce any user-facing change?
Better plans
How was this patch tested?
Existing tests