Optimize filter condition with switch case

[maswin]

Test plan - Unit tests + verifier run on internal queries

== RELEASE NOTES ==

General Changes
* Added a new Optimizer SimplifySwitchExpression which simplifies switch cases in filter condition

The DB visualization tools such as looker & DbVisualizer generates sub-optimal queries when a selected column has switch case and a filter is applied on top of it. For instance it generates queries such as:

SELECT case behavior_code when 1 then 'good' when 2 then 'bad' else 'neutral' from students where (case behavior_code when 1 then 'good' when 2 then 'bad' else 'neutral') = 'good';

But this query can be simplified into:

SELECT case behavior_code when 1 then 'good' when 2 then 'bad' else 'neutral' from students where behavior_code = 1;

Without this simplification, the query misses out a lot of other optimizations (i.e, ORC indexes) and becomes extremely slow.

Since these are generated queries based on the UI, the user doesn't have much control on them.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: maswin / name: Alagappan Maruthappan (f085fc9eef45da04d6952ae35467a17f5859edfa)

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Alagappan Maruthappan (47ea97a23a8eed19039358a6ca8fa784584308d9)

[kaikalur]

An interesting way to do this would be to flip WHEN/THEN (by appropriately adding equals to the THEN) and calling RowExpresionInterpreter. Like take the example and create a new expression:

case when 'good' = 'good' then behavior_code=1 when 'good'= 'bad' then behavior_code=2  when 'good' =  'neutral' then not(behavior_code=1  OR behavior_code=2 ) end

and call the RowExpressionInterpter on it. That should be more robust.

[kaikalur]

@rongrong Check it out

[kaikalur]

In fact, this can be generalized to any relational expression where one side is case - just push relation to the THEN parts, flip them and evaluate it. ELSE part is simply NOT of OR of all the new THEN parts.

[maswin]

What about the case when 2 WHEN clauses match the THEN part

(CASE 
  WHEN column_val = 1 THEN 'a'   // Matching
  WHEN column_val = 2 THEN 'a'   // Matching
  WHEN column_val = 3 THEN 'b'
ELSE 'c') = 'a'

flipping this will make it:

CASE 
  WHEN 'a'='a' THEN column_val = 1
  WHEN 'a'='a' THEN column_val = 2
  WHEN 'a'='b' THEN column_val = 3
  WHEN 'a'='c' THEN !(column_val = 1 OR column_val = 2 OR column_val = 3)
ELSE false

Since in switch case when first condition is matched we use it and break of, RowExpressionInterpter will simplify this to:
column_val = 1
But the right simplification is
column_val = 1 OR column_val = 2

[maswin]

It needs to be converted to a series of OR clauses:

(CASE 
  WHEN column_val = 1 THEN 'a'   // Matching
  WHEN column_val = 2 THEN 'a'   // Matching
  WHEN column_val = 3 THEN 'b'
ELSE 'c') = 'a'

above expression gets transformed to:

('a'='a' AND column_val = 1) OR 
('a'='a' AND column_val = 2) OR 
('b'='a' AND column_val = 3) OR 
('c'='a' AND !(column_val = 1 OR column_val = 2 OR column_val = 3))

RowExpressionInterpter can transform this to a simpler expression

column_val = 1 OR column_val = 2

This handles lot of other extreme cases too,
i.e one of the THEN clause and ELSE clause matches

(CASE 
  WHEN column_val = 1 THEN 'a'   // Matching
  WHEN column_val = 2 THEN 'b' 
ELSE 'a'      // Matching
) = 'a' 

get simplified to

column_val != 2

[kaikalur]

It needs to be converted to a series of OR clauses:

(CASE 
  WHEN column_val = 1 THEN 'a'   // Matching
  WHEN column_val = 2 THEN 'a'   // Matching
  WHEN column_val = 3 THEN 'b'
ELSE 'c') = 'a'

above expression gets transformed to:

('a'='a' AND column_val = 1) OR 
('a'='a' AND column_val = 2) OR 
('b'='a' AND column_val = 3) OR 
('c'='a' AND !(column_val = 1 OR column_val = 2 OR column_val = 3))

RowExpressionInterpter can transform this to a simpler expression

column_val = 1 OR column_val = 2

This handles lot of other extreme cases too, i.e one of the THEN clause and ELSE clause matches

(CASE 
  WHEN column_val = 1 THEN 'a'   // Matching
  WHEN column_val = 2 THEN 'b' 
ELSE 'a'      // Matching
) = 'a' 

get simplified to

column_val != 2

Hmm, yeah I'm usually not a fan of OR expressions because they are harder to pushdown. If we are going to stick to doing it only for cases when all THEN parts are constant/literal, we can just build a map THEN -> WHEN[] and the generate an OR for each.

[maswin]

Hmm, yeah I'm usually not a fan of OR expressions because they are harder to pushdown. If we are going to stick to doing it only for cases when all THEN parts are constant/literal, we can just build a map THEN -> WHEN[] and the generate an OR for each.

Yeah, it is difficult to pushdown, but atleast OR seems better to deal with than CASE expressions for further Optimizations. Was assuming if not a non deterministic function is involved in the expression to do this optimization. Since conversion from CASE to OR doesn't change the final evaluation result of the predicate and scenarios like concat('ab'. 'cd') = 'abcd' will also be handled

[kaikalur]

Hmm, yeah I'm usually not a fan of OR expressions because they are harder to pushdown. If we are going to stick to doing it only for cases when all THEN parts are constant/literal, we can just build a map THEN -> WHEN[] and the generate an OR for each.

Yeah, it is difficult to pushdown, but atleast OR seems better to deal with than CASE expressions for further Optimizations. Was assuming if not a non deterministic function is involved in the expression to do this optimization. Since conversion from CASE to OR doesn't change the final evaluation result of the predicate and scenarios like concat('ab'. 'cd') = 'abcd' will also be handled

Actually CASE is sequential and OR is not so but I guess it won't matter here if we are checking for constants only. Issue with OR is the whole NULL handling. So if we can do CASE, we should.

Also like I said see if you can generalize for all relational operators.

[kaikalur]

Also please add some explicit end-to-end tests in hive connector to see the impact of this optimization.

[kaikalur]

I think it's best to check that otherExpression is actually a constant so it is clearly beneficial without too many issues with non-determinism etc

[maswin]

We have scenarios such as

(CASE
    WHEN (replace(column_name, 'prefix_string')) = 'dept1' THEN 'V1'
    WHEN (replace(column_name, 'prefix_string')) = 'dept2'  THEN 'V2
    ELSE (replace(column_name, 'prefix_string'))
    END
) = UPPER('v1');

In this case checking if it is just Constant Expression would be problematic.

[maswin]

One problem we see with CASE expression is that it doesn't extract out the Column Domain while picking table layout. Switching it to AND/OR cases helps achieve it. Even if there is a column reference present in the equals side

(CASE
    WHEN (replace(column_name, 'prefix_string')) = 'dept1' THEN 'V1'
    WHEN (replace(column_name, 'prefix_string')) = 'dept2'  THEN 'V2
    ELSE 'V3'
    END
) = column_name_2;

Above expression gets converted into a cumbersome AND/OR expression since column value's equality with THEN value cannot be evaluated and the expression won't be further simplified, but still it helps RowExpressionInterpretter/Optimizers in extracting column domain for the table from that expression. i.e, column_name_2 domain is set as [ V1, V2, V3].

[kaikalur]

These things can lead to subtle bugs. And also if the other side is a function call pushing it into the CASE can make it evaluate many times. We should not need to do random stuff like upper(..). But maybe extend it to just constant or a variable/field reference/input expression.

[kaikalur]

Do you really need this class?

[maswin]

Simplified without the class.

[kaikalur]

Why do you need to check for this?

[maswin]

Both SearchedCase and SimpleCase expression both have the same format in RowExpresion, if its a SimpleCase Expression, in RowExpresion in place of the case operand we would have ConstantExpression(true). So I am performing the check to avoid doing true = when condition.
Similar to this:

presto/presto-main/src/main/java/com/facebook/presto/sql/gen/SwitchCodeGenerator.java

Line 83 in 123c2ab

boolean searchedCase = (value instanceof ConstantExpression && ((ConstantExpression) value).getType() == BOOLEAN &&

[kaikalur]

In fact, we want to check either all of the THEN and ELSE are constant or the otherExpression is constant

[kaikalur]

And we normally start by disabling the feature by default

[maswin]

Done.

[maswin]

Also please add some explicit end-to-end tests in hive connector to see the impact of this optimization.

Can you please point out where these tests exists? Internally we were able to see queries that ran for over 10 minutes to run in less than 10 seconds.

[kaikalur]

We also need more end to end tests, not just unit test. Look at AbstractTestQueries - adding there will test with mutliple connectors which will be good.

[kaikalur]

equals is a confusing name! Make it createEquals

[maswin]

Done

[kaikalur]

Add a helper method for this conditional so the logic is clear.

[maswin]

Refactored the part.

[kaikalur]

Also fix the test failures

[maswin]

We also need more end to end tests, not just unit test. Look at AbstractTestQueries - adding there will test with mutliple connectors which will be good.

Let me know if more cases needs to be handled in AbstractTestQueries

[kaikalur]

@maswin where are we at with this PR? This could be useful for one of our usecases so just wanted to check if there is anything else missing. Thanks!

[maswin]

@maswin where are we at with this PR? This could be useful for one of our usecases so just wanted to check if there is anything else missing. Thanks!

We have handled the case where there is a Cast done on top of the expression since most of the time to match the lhs and rhs expression type expressions are cast with a super data type. All other mentioned review comments are fixed and updated.

[kaikalur]

Thank you! Added @rschlussel for further review and hopefully merging.

[rschlussel]

I added a bunch of comments to RewriteCaseExpressionPredicate, but I actually wonder if it would be better to do this with the other expression rewrites at the beginning of optimization using an ExpressionRewriteRuleSet

[rschlussel]

would be good to add a note about why this limitation exists (am I correct that it's because case expressions are ordered, so you need the expressions to be disjunct (can only have one be true) to replace it with an or?

[maswin]

I have rewritten the description with the reasoning for it.

[rschlussel]

why are there restrictions for this and the rhs expressions beyond just requiring a deterministic function for the value? Is it just less useful in those cases? also why doesn't the rhs restriction also allow column references?

[maswin]

Will we be able to evaluate if the conditions are disjunct at query planning time if there are column references at RHS?

And most of the time it is the data visualization tool that generates such query, which usually is a column at LHS and constant at RHS. We have handled the most common case.

[rschlussel]

should there also be some restriction on the number of cases?

[maswin]

Internally we saw many case statements with ~30-40 WHEN conditions and most of the time some WHEN return value gets matched to the final equals value and most of the OR clause gets removed in final expression, especially the last else statement. So I am not sure if this is a needed restriction.

[rschlussel]

If the LHS are the same, why do the RHS need to be unique? Wouldn't it just be a duplicate?

[maswin]

If we do the rewrite without this check (the RHS is not unique)

(case 
when col1=1 return 'a'
when col1=1 return 'b'
else 'c') = 'b'

will get simplified to

(col1=1 AND 'a'='b') OR
(col1=1 AND 'b'='b') OR
('c' = 'b' AND col1<>1 AND col2<>2)

'a'='b' condition would evaluate to false, and will get simplified to
col1=1
But this is a wrong simplification. If col1=1, it would have gone to first WHEN clause in CASE statement and returned 'a' which wouldn't be equal to 'b'. Our simplified condition allows rows with col1=1 to pass but in actual case it should not pass.

[rschlussel]

use an immutableListBuilder

[maswin]

Done.

[rschlussel]

always use immutableCollections

[maswin]

Done.

[rschlussel]

you don't need to compare against caseExpressionRewriteDisabled. assertQuery compares the results against another DB. it ignores the session completely for creating the expected results

[maswin]

Didn't realize that. Fixed.

[rschlussel]

this shouldn't be necessary

[maswin]

Done.

[rschlussel]

what is going on with the second half of this test here?

[maswin]

The second half of the test checks the Optimized version of the expression. Most of the times the result=value condition gets evaluated to false and a simplified final expression would be produced. That simplification is not done by the rewriter and it is against unit testing principle to test it here, but it helped in understanding and developing the Rewriter better so retained them in the final commit.

[rschlussel]

I think it would be better to remove this second half of the test. I think it makes the test more complex/confusing and doesn't add much value to the code coverage.

[rschlussel]

love the thorough test cases!

[maswin]

I added a bunch of comments to RewriteCaseExpressionPredicate, but I actually wonder if it would be better to do this with the other expression rewrites at the beginning of optimization using an ExpressionRewriteRuleSet

We wanted the setting to be disabled initially, couldn't find disabling in ExpressionRewriteRuleSet/RowExpressionRewriteRuleSet. But yeah, that might help it extending to all types of Nodes.

[maswin]

I added a bunch of comments to RewriteCaseExpressionPredicate, but I actually wonder if it would be better to do this with the other expression rewrites at the beginning of optimization using an ExpressionRewriteRuleSet

We wanted the setting to be disabled initially, couldn't find disabling in ExpressionRewriteRuleSet/RowExpressionRewriteRuleSet. But yeah, that might help it extending to all types of Nodes.

I have now made an additional commit that adds the ability for RowExpressionRewriterRuleSet to be disabled if required and modified the RewriteCaseExpressionPredicate to extend RowExpressionRewriteRuleSet. This enables it to optimize filter expression in JOIN condition too.

[rschlussel]

add a checkArgument that expression is a cast expression and its argument is a case expression

[maswin]

Done.

[rschlussel]

I think it would be better to remove this second half of the test. I think it makes the test more complex/confusing and doesn't add much value to the code coverage.

[rschlussel]

Thank you!

[rschlussel]

@kaikalur can you review/approve? Merging is blocked since you are still marked as requesting changes.

[kaikalur]

Are we ready to merge this?

[rschlussel]

yup. I'll merge now!