[SPARK-25141][SQL][TEST] Modify tests for higher-order functions to check bind method.

[ueshin]

What changes were proposed in this pull request?

We should also check HigherOrderFunction.bind method passes expected parameters.
This pr modifies tests for higher-order functions to check bind method.

How was this patch tested?

Modified tests.

[ueshin]

cc @mn-mikke @mgaido91

[mgaido91]

I am not sure about this. I think that the bind method itself is tested with the end-to-end tests which have been added. This complicates quite this part which is meant only to test the execution of the functions. I don't see much value added by this tests, but I may miss something. Can you enlighten me @ueshin ? Thanks.

[ueshin]

Actually one of my motivations is to prevent a mistake like #22126. To create a test, we needed to do the same thing in bind. The other is I wanted to check the exact values each function passes to bind function as in the description, which we can't know with the end-to-end tests.

[mgaido91]

I see now, thanks. The only thing which I am a bit concerned about is the complexity introduced by this change, in the sense that it is relying very much on the bind implementation of each expression. I think it would be great if we could generalize it.

Can we add a generic function similar to the createLambda in the analyzer for this?

[mgaido91]

sorry, I wasn't very specific in my previous comment. I meant adding a method like:

def validateBinding(e: HigherOrderFunction): Expression = {
    e.bind { case (f: LambdaFunction, dataTypes) =>
      f.arguments.zip(dataTypes).foreach {
        case (arg, (dt, n)) =>
          assert(arg.dataType == dt)
          assert(arg.nullable == n)
      }
      f
    }
  }

and then validate all the expressions, so use it like:

validateBinding(ArrayTransform(....))

What do you think about this?

[SparkQA]

Test build #94883 has finished for PR 22131 at commit 8154bf4.

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

[ueshin]

Sounds good. I just hope we will never miss to wrap new functions with it. Thanks!

[mgaido91]

yes, this is my worry too... But I don't have any suggestion for that.... If you have anyone, that's great :)

[mgaido91]

LGTM

[mn-mikke]

LGTM too

[SparkQA]

Test build #94887 has finished for PR 22131 at commit dec6978.

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

[SparkQA]

Test build #94888 has finished for PR 22131 at commit 22c3f48.

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

[SparkQA]

Test build #94891 has finished for PR 22131 at commit 8a844b7.

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

[ueshin]

@mgaido91 @mn-mikke On second thought, how about this?
If you don't like it, I'll revert it soon.

[SparkQA]

Test build #94919 has finished for PR 22131 at commit 6f9660d.

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

[mgaido91]

I preferred the previous one (but yes, definitely good to add the check about the args size), but I am fine with this one too. Thanks @ueshin

[ueshin]

Thanks! I'd use this one. merging to master.