Add support for StringConcatFactory.makeConcatWithConstants (#9555)

[seanprime7]

This PR adds support for StringConcatFactory.makeConcatWithConstants.
4 of the existing unit tests started failing with JDK11 as the bytecode generation target due to the introduction of StringConcatFactory.makeConcatWithConstants in JDK9.
See #9555 for more details about the failures.

It resolves #9555

[seanprime7]

@abellina

[abellina]

This looks good to me! It needs copyright updated, but the catalyst it produces matches what we did in JDK8/2.12.

[abellina]

One thing separate from this PR we might consider is to try and simplify the concat catalyst expressions. With JDK8/scala 2.12 and with JDK11/scala 2.13 I see the same thing:

== Physical Plan ==
AdaptiveSparkPlan isFinalPlan=false
+- Project [x#2856, y#2857, z#2858, if (z#2858) concat(, , x#2856,  , y#2857, @@@,  true) else concat(, , y#2857,  , x#2856, !!!,  false) AS new#2862]
   +- Exchange SinglePartition, REPARTITION_BY_NUM, [id=#883]
      +- LocalTableScan [x#2856, y#2857, z#2858]

I think the concats cold be slightly simpler to remove that first empty string:

concat(x#2856, ' ', y#2857, '@@@',  ' true')

The code we are generating does the right thing, I am just saying it could be improved. @seanprime7 would this be an easy change? And it's ok for us to file an issue and do as a follow on, especially since it was already there before.

[seanprime7]

One thing separate from this PR we might consider is to try and simplify the concat catalyst expressions. With JDK8/scala 2.12 and with JDK11/scala 2.13 I see the same thing:

== Physical Plan ==
AdaptiveSparkPlan isFinalPlan=false
+- Project [x#2856, y#2857, z#2858, if (z#2858) concat(, , x#2856,  , y#2857, @@@,  true) else concat(, , y#2857,  , x#2856, !!!,  false) AS new#2862]
   +- Exchange SinglePartition, REPARTITION_BY_NUM, [id=#883]
      +- LocalTableScan [x#2856, y#2857, z#2858]

I think the concats cold be slightly simpler to remove that first empty string:

concat(x#2856, ' ', y#2857, '@@@',  ' true')

The code we are generating does the right thing, I am just saying it could be improved. @seanprime7 would this be an easy change? And it's ok for us to file an issue and do as a follow on, especially since it was already there before.

I've made a change not to generate Concat ops with empty strings in it. If you want to further simplify the expression, you can add more simplification rules to CatalystExpressionBuilder.scala.

[abellina]

The optimization (for empty space concat) only went to scala 2.13, but that should be fine (the old code has been there for a long time).

[abellina]

build

[abellina]

@seanprime7 so I tried the optimization, but the catalyst plan looks the same?

== Physical Plan ==
AdaptiveSparkPlan isFinalPlan=false
+- Project [x#2856, y#2857, z#2858, if (z#2858) concat(, x#2856,  , y#2857, @@@ true) else concat(, y#2857,  , x#2856, !!! false) AS new#2862]
   +- Exchange SinglePartition, REPARTITION_BY_NUM, [id=#883]
      +- LocalTableScan [x#2856, y#2857, z#2858]

I'd rather split that off to a different issue, and back-out 047b416, we can take a look at it later for both scala 2.12 and 2.13.

Thoughts?

[seanprime7]

047b416 has been backed out

[abellina]

build

[abellina]

thanks @seanprime7