Optimize filter condition with CASE predicate - handle null cases#18231
Optimize filter condition with CASE predicate - handle null cases#18231maswin wants to merge 1 commit intoprestodb:masterfrom
Conversation
|
Like I said - just coalesce instead of IS NULL OR etc. COALESCE is better. |
|
User facing documentation is too technical with internal details. Need to rework that. |
Do you mean to rewrite in this form: the main reason we want this rewrite to happen is for Presto to construct column domains so that the execution is faster. Currently only for simple AND/OR predicates column domains are constructed. RowExpressionDomainTranslator wont be able to construct column domain if any function (COALESCE) is used. When I use COALESCE as mentioned above, the ScanFilter looks like this: No column domains in the table scan layout. by rewriting using IS NULL we get: by using IS NULL we get column domain as Above example is when result matches one of the when clause, even if it matches the else clause we would get a domain like: column domain: |
Wow! that's not good. OK sounds good. |
20aa571 to
419c620
Compare
Updated. |
There was a problem hiding this comment.
Can you link https://cloud.google.com/looker/docs/reference/param-field-case here? Also can you make it lowercase (case instead of CASE). lookml is all lowercase and so they only refer it as case.
419c620 to
413f85a
Compare
413f85a to
1569ea5
Compare
steveburnett
left a comment
There was a problem hiding this comment.
This PR is old.
Local doc build failed with
"python3 -m sphinx -b html -n -d target/doctrees -j auto src/main/sphinx target/html
Running Sphinx v4.4.0
Sphinx version error:
The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version.
make: *** [html] Error 2"
git merge master - the usual solution to this build problem related to PR #21708 - failed:
"Auto-merging presto-docs/src/main/sphinx/admin/properties.rst
Auto-merging presto-main/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java
Auto-merging presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RewriteCaseExpressionPredicate.java
CONFLICT (content): Merge conflict in presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RewriteCaseExpressionPredicate.java
Auto-merging presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
Automatic merge failed; fix conflicts and then commit the result."
Please update the PR, then re-request my review of the docs.
This is an extension to the following PR: #17065
The following conditions were not handled previously:
when col is NULL, the expression should evaluate to TRUE since NULL will match none of the WHEN clause and reach the else block. Else case will return 'c' which is equal to 'c'. But the previous simplification will result in:
which will be simplified to
('c'='c' AND (col != 1 AND col !=2))->col != 1 AND col !=2but this wouldn't evaluate to TRUE due to the way NULL are handled in sql. This will result in NULL.
So we have added NULL checks in the rewrite, and the new rewrite would be of the form:
which will be simplified to
('c'='c' AND ((col IS NULL) OR (col != 1 AND col !=2)))->(col IS NULL) OR (col != 1 AND col !=2))this will rightly get evaluated to TRUE.
Result matching with any other when clause result case is also handled. Tests are written for those cases in AbstractTestQueries. e.g:
NULL value in col will result in returning else result 'c' which is != 'b' there by evaluating to FALSE. Above expression will be rewritten as
simplified as
'b'='b' AND col IS NOT NULL AND col = 2->col IS NOT NULL AND col = 2which will evaluate to FALSE.
If the expression is of form
(case when col1=CAST(1 as SMALLINT) then 'case1' when col1=CAST(1 as TINYINT) then 'case2' else 'default' end) = 'case1'the rewrite should not happen since RHS values in operand are not unique, but currently it happens since we were checking if the ConstantExpression are same. Modified it to evaluate the expression and then perform the check.
Also added documentation for the setting.