[SPARK-23628][SQL] calculateParamLength should not return 1 + num of epressions

[mgaido91]

What changes were proposed in this pull request?

There was a bug in calculateParamLength which caused it to return always 1 + the number of expressions. This could lead to Exceptions especially with expressions of type long.

How was this patch tested?

added UT + fixed previous UT

[mgaido91]

cc @cloud-fan @kiszk @viirya

[kiszk]

I may be wrong, but I do not find algorithm changes in calculateParamLength.

I believe that the original implementation is correct. This is because calculateParamLength returns the number of Java stack operands in JVM. Long or Double requires 2 stack operands to pass a value. In other words, we can pass up to 127 long values in a method argument list.

WDYT?

[mgaido91]

@kiszk the logic is not changed, but the previous implementation is wrong. Let's go through an example to show more clearly this. Let's assume we have a long nullable variable. Substituting the values, the previous code becomes:

scala> 1 + "long" match {
     |  case "int" | "long" => 2
     |  case _ => 1
     | }
res0: Int = 1

Instead it should return 3, as the new code does. This is because 1 + "long" is executed before the match.... So previously this returned always 1.

Is it clear now?

[kiszk]

Yeah, thank you for your explanation. Good catch.

[SparkQA]

Test build #88094 has finished for PR 20772 at commit bd89841.

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

[cloud-fan]

good catch! merging to master!

[cloud-fan]

@mgaido91 do you mind to open a new PR for 2.3? it conflicts. thanks!

[hvanhovell]

@mgaido91 this is good catch!

LGTM. Merging to master/2.3. Thanks!

[hvanhovell]

@cloud-fan you beat me to it :)

[cloud-fan]

haha, 1 minute before you :)

[mgaido91]

Thanks @cloud-fan @hvanhovell, I'll create a backport for 2.3 tomorrow morning CEST :)

[viirya]

Good catch! Thanks! @mgaido91

[rednaxelafx]

Just wanna leave a note here that the Janino side of the bug has been fixed:
janino-compiler/janino#46 (comment)
janino-compiler/janino@a373ac9
With this fix, Janino will correctly throw an exception upon invalid parameter size instead of silently allowing it to pass compilation (which leads to ClassFormatError later when the generated class is loaded by the JVM).