[SPARK-26107][SQL] Extend ReplaceNullWithFalseInPredicate to support higher-order functions: ArrayExists, ArrayFilter, MapFilter

[rednaxelafx]

What changes were proposed in this pull request?

Extend the ReplaceNullWithFalse optimizer rule introduced in SPARK-25860 (#22857) to also support optimizing predicates in higher-order functions of ArrayExists, ArrayFilter, MapFilter.

Also rename the rule to ReplaceNullWithFalseInPredicate to better reflect its intent.

Example:

select filter(a, e -> if(e is null, null, true)) as b from (
  select array(null, 1, null, 3) as a)

The optimized logical plan:
Before:

== Optimized Logical Plan ==
Project [filter([null,1,null,3], lambdafunction(if (isnull(lambda e#13)) null else true, lambda e#13, false)) AS b#9]
+- OneRowRelation

After:

== Optimized Logical Plan ==
Project [filter([null,1,null,3], lambdafunction(if (isnull(lambda e#13)) false else true, lambda e#13, false)) AS b#9]
+- OneRowRelation

How was this patch tested?

Added new unit test cases to the ReplaceNullWithFalseInPredicateSuite (renamed from ReplaceNullWithFalseSuite).

[rednaxelafx]

cc @aokolnychyi : I'd like to propose renaming the rule you introduced to add a -InPredicate suffix, because obviously we can't replace arbitrary nulls with false, but only the ones that are going to be directly used in a boolean predicate context (e.g. If(cond, _, _), Filter etc that you've already nicely identified). Does that make sense to you?

BTW thank you very much for introducing that rule. It's really neat!

[SparkQA]

Test build #98975 has finished for PR 23079 at commit 710c886.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[aokolnychyi]

@rednaxelafx I am glad the rule gets more adoption. Renaming also makes sense to me.

Shall we extend ReplaceNullWithFalseEndToEndSuite as well?

[aokolnychyi]

Test cases for ArrayFilter and ArrayExists seem to be identical. As we have those tests anyway, would it make sense to cover different lambda functions?

[rednaxelafx]

Actually I intentionally made all three lambda the same (the MapFilter one only differs in the lambda parameter). I can encapsulate this lambda function into a test utility function. Let me update the PR and see what you think.

[rednaxelafx]

Updated.

[cloud-fan]

shall we add a withNewFunctions method in HigherOrderFunction? Then we can simplify this rule to

case f: HigherOrderFunction => f.withNewFunctions(f.functions.map(replaceNullWithFalse))

[rednaxelafx]

I'm not sure if that's useful or not. First of all, the replaceNullWithFalse handling doesn't apply to all higher-order functions. In fact it only applies to a very narrow set, ones where a lambda function returns BooleanType and is immediately used as a predicate. So having a generic utility can certainly help make this PR slightly simpler, but I don't know how useful it is for other cases.
I'd prefer waiting for more such transformation cases to introduce a new utility for the pattern. WDYT?

[cloud-fan]

ah i see. Sorry I missed it. Then it's safer to use a white-list here.

[SparkQA]

Test build #98996 has finished for PR 23079 at commit 6646a96.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
• class ReplaceNullWithFalseInPredicateEndToEndSuite extends QueryTest with SharedSQLContext

[aokolnychyi]

LGTM as well.

[cloud-fan]

thanks, merging to master!