Cardinality of filter Optimization

[kaikalur]

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* We now optimize the following patterns
  * CARDINALITY(FILTER(a, x -> f(x)))   ===>  REDUCE(a, cast(0 as bigint), (s,x) -> case when f(x) then s + 1 else s end, s->s)
  * CARDINALITY(FILTER(a, x -> f(x))) > 0   ===> REDUCE(a, false, (s,x)->case when s then s else f(x) end, s -> s)
  * CARDINALITY(FILTER(a, x -> f(x))) = 0   ===> REDUCE(a, false, (s,x)->case when s then s else f(x) end, s -> NOT s)
  * CARDINALITY(FILTER(a, x -> f(x))) <comparison> n   ===> REDUCE(a, cast(0 as bigint), (s,x)->case when s <comparison> n then s case when f(x) s + 1 else s end, s -> s <comparison> n)

It's controlled by session property: simplify_array_operations with default set to true.
* ...

Hive Changes
* ...
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

[linux-foundation-easycla]

One or more committers are not authorized under a signed CLA as indicated below. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

• ✅ Sreeni Viswanadha (e06cb87)

• ❌ Grzegorz Kokosiński (1a7a3c1)

[wenleix]

Nice idea of expression optimization. @shixuan-fan , @hellium01 wondering if you can help with a first round of review? -- This helps with us to enlarge the reviewer pool and get you familiar with more part of code~

Speaking of commit convention, in general in Presto code based we always do rebase and don't do merge.

[shixuan-fan]

This is a clever optimization :)

[shixuan-fan]

Maybe we could use getOnlyElement(functionCall.getArguments()) assuming cardinality should only have one argument.

I'm not entirely sure about this because if cardinality argument count ever changes, we will fail the query, and the current code would simply ignore this optimization. Let's see what others think.

[hellium01]

ditto to this. cardinality should always has single argument, if not, we should throw.

[kaikalur]

I don't like throwing from the optimizer. This is just opportunistic optimization and be defensive and have the exact pattern that you want to match,

[rongrong]

Theoretically you don't need to check that here. Function resolution would have failed if arguments size is not 1. Agree with @hellium01's earlier point that this should be added to RowExpressionRewriteRuleSet. Also when you do that, please use proper API (add isCardinalityFunction isFilterFunction to StandardFunctionResolution) rather than relying on name (CallExpression's name is only used for display purpose)

[shixuan-fan]

Let's just name this whenClauses.

[shixuan-fan]

This is inputFunction I believe?

[shixuan-fan]

Let's also revamp this method to be in the following layout to facilitate review process, also, let's also rename the variables following the comments in simplifyCardinalityOfFilterComparedToZero:

- create cast (not required if it could be inlined)
- create case statement
- create input function
- create output function
- create reduce function

[shixuan-fan]

Let's rename the commit title and PR title to "Optimize cardinality of filter expression" and put benchmark results in commit message. Also, is there a benchmark class (like BenchmarkArrayAggregation) that you are using?

[hellium01]

This is a really smart optimization :). However, we probably need to port it to RowExpressionRewriteRuleSet as ExpressionRewriteRuleSet is deprecating...

[hellium01]

Can you rebase and squash the unrelated commits?

[hellium01]

Also, please follow: https://github.com/prestodb/presto/wiki/Presto-Development-Guidelines
If you are using IntelliJ (which I highly recommend), definitely import the style file from here: https://github.com/airlift/codestyle.

[kaikalur]

I thought I had the airift code style. I will doublecheck

[hellium01]

SIMPLIFY_ARRAY_OPERATIONS probably is a little too general? probably OPTIMIZE_FILTER_CARDINALITY will be more specific for user?

[rongrong]

Alternative question to ask: Do you want this flag to guard all future optimization rules that would be added to arrays?

[kaikalur]

Alternative question to ask: Do you want this flag to guard all future optimization rules that would be added to arrays?

Yeah - I was thinking about that. It's a tricky balance between having too many flags and controlling what is enabled. Since most of these optimizations are opportunistic and hopefully don't need to be turned off frequently, I thought one is just enough. But i can change it if there is a strong reason.

[hellium01]

Probably indicate it will optimize cardinality(filter(a, x->f(x)) expression in the session description will be more clear to user?

[kaikalur]

Most users won't even understand what that means. This flag is only for any unforeseen bugs.

[hellium01]

ditto to this. cardinality should always has single argument, if not, we should throw.

[hellium01]

You can use VariableAllocator to make sure there is no confict.

[hellium01]

We don't have to worry about this if we are operating on RowExpression :)

[hellium01]

From another perspective, the reason user will write cardinality(filter(...)) might be because we lack function do:

any(a, predicate)
count_on(a, predicate)
...

thus probably another solution is to create such functions?

[kaikalur]

Yes that will be a next step but we have several major pipelines using this construct so this will have immediate benefit.

[kaikalur]

Let's rename the commit title and PR title to "Optimize cardinality of filter expression" and put benchmark results in commit message. Also, is there a benchmark class (like BenchmarkArrayAggregation) that you are using?

This is a tricky one to measure because it's not an operator. We can add a benchmark test that loops through this operation a thousand times but not sure.

[wenleix]

@kaikalur : The first optimization rule is definitely good to avoid unnecessary array construction. I am wondering if the second and third optimization rule is necessary? , i.e.

  * CARDINALITY(FILTER(a, x -> f(x))) > 0   ===> REDUCE(a, false, (s,x)->case when s then s else f(x) end, s -> s)
  * CARDINALITY(FILTER(a, x -> f(x))) = 0   ===> REDUCE(a, false, (s,x)->case when s then s else f(x) end, s -> NOT s)

In my opinion we should just backport any_match from PrestoSQL: https://prestosql.io/docs/current/functions/array.html#any_match .

And I am not sure if I understand the fourth optimization rule will be helpful, i.e.

  * CARDINALITY(FILTER(a, x -> f(x))) <comparison> n   ===> REDUCE(a, cast(0 as bigint), (s,x)->case when s <comparison> n then s case when f(x) s + 1 else s end, s -> s <comparison> n)

With the first optimization rule, we already avoid unnecessary array construction. And even with this rule we still need to go through the whole array. And my intuition is the overhead of reduce function itself will dominate and for now we probably don't need to optimize for this "+1" operation? :)

[kaikalur]

In my opinion we should just backport any_match from PrestoSQL: https://prestosql.io/docs/current/functions/array.html#any_match .

Technically, anymatch works only for > 0, for = 0 it doesn't short circuit to return false on the first match.

[kaikalur]

nd my intuition is the overhead of reduce function itself will dominate and for now we probably don't need to optimize for this "+1" operation? :)

It will stop on the first time the comparison succeeds so if you have a large array, it will benefit

[wenleix]

all_match(), any_match(), and none_match() are being backported in #13734

[wenleix]

@kaikalur :

Technically, anymatch works only for > 0, for = 0 it doesn't short circuit to return false on the first match.

We have any_match, all_match and none_match 😄 . I believe it should cover this case ~

[kaikalur]

@kaikalur :

Technically, anymatch works only for > 0, for = 0 it doesn't short circuit to return false on the first match.

We have any_match, all_match and none_match 😄 . I believe it should cover this case ~

Too much code, yuck! I would rather have a better short-circuiting reducer. But may be I will back port them for now.

[kaikalur]

@kaikalur :

Technically, anymatch works only for > 0, for = 0 it doesn't short circuit to return false on the first match.

We have any_match, all_match and none_match 😄 . I believe it should cover this case ~

Too much code, yuck! I would rather have a better short-circuiting reducer. But may be I will back port them for now.

And also NonMatch just calls AnyMatch! I'm not backporting that. It's not a performant implementation.

[rongrong]

We should also add a SQL function for this one!

[rongrong]

Looking at the implementation I think it would be much simpler if we add a SQL function and just rewrite it with the SQL function. Maybe you want to switch to work on supporting SQL function as builtin functions instead? 🤣

[rongrong]

Alternative question to ask: Do you want this flag to guard all future optimization rules that would be added to arrays?

[rongrong]

Theoretically you don't need to check that here. Function resolution would have failed if arguments size is not 1. Agree with @hellium01's earlier point that this should be added to RowExpressionRewriteRuleSet. Also when you do that, please use proper API (add isCardinalityFunction isFilterFunction to StandardFunctionResolution) rather than relying on name (CallExpression's name is only used for display purpose)

[kaikalur]

Regarding doing it for RowExpressions - I would say no. The idea is to move these rewrites up the stack to just after parse/resolve and eventually they will be string/template based ones - parse -resovle-(unparse(rewrite)-parse-resolve)* so people get to see the query structure to do more interesting. I'm doing this one for now because it's widely used pattern.