[SPARK-40385][SQL] Fix interpreted path for companion object constructor

[eejbyfeldt]

What changes were proposed in this pull request?

Fixes encoding of classes that uses companion object constructors in the interpreted path. Without this change the that is added in this change would fail with

...
  Cause: java.lang.RuntimeException: Error while decoding: java.lang.RuntimeException: Couldn't find a valid constructor on interface org.apache.spark.sql.catalyst.ScroogeLikeExample
newInstance(interface org.apache.spark.sql.catalyst.ScroogeLikeExample)
  at org.apache.spark.sql.errors.QueryExecutionErrors$.expressionDecodingError(QueryExecutionErrors.scala:1199)
...

As far as I can tell this bug has existed since the initial implementation in SPARK-8288 #23062

The existing spec that tested this part of the code incorrectly provided an outerPointer which hid the bug from that test.

Why are the changes needed?

Fixes a bug, the new spec in the ExpressionsEncoderSuite shows that this is in fact a bug.

Does this PR introduce any user-facing change?

Yes, it fixes a bug.

How was this patch tested?

New and existing specs in ExpressionEncoderSuite and ObjectExpressionsSuite.

[AmplabJenkins]

Can one of the admins verify this patch?

[HyukjinKwon]

Shall we create a new test case?

[eejbyfeldt]

I am not sure I understand what the new and the old test case would be. My understanding is that outerPointer is only for inner classes but ScroogeLikeExample is not an inner class. So the old test case was incorrect and only works because the old code dropped the outer param by incorrectly calling tail on the param list. So this change is necessary for this spec not to fail.

[HyukjinKwon]

Hm, I wonder if we should add a test for outer point and see if that still works ..

[eejbyfeldt]

So normal inner classes (that uses outer pointer) are tested here in the ExpressionEncoderSuite:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala

Lines 229 to 233 in ae08787

	case class InnerClass(i: Int)
	productTest(InnerClass(1))
	encodeDecodeTest(Array(InnerClass(1)), "array of inner class")
	encodeDecodeTest(Array(Option(InnerClass(1))), "array of optional inner class")

and here in the ObjectExpressionsSuite:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala

Lines 408 to 417 in ae08787

	// Inner class case test
	val outerObj = new Outer()
	val newInst2 = NewInstance(
	cls = classOf[outerObj.Inner],
	arguments = Literal(1) :: Nil,
	inputTypes = Nil,
	propagateNull = false,
	dataType = ObjectType(classOf[outerObj.Inner]),
	outerPointer = Some(() => outerObj))
	checkObjectExprEvaluation(newInst2, new outerObj.Inner(1))

so such cases are covered.

Are you saying that we should add a test case for a class that only have an companion apply constructor? Trying to do something like that will not work in either this branch or master. This is because companion objects of inner classes are not singletons and the codegen will fail with that "MODULE$" is neither a method, a field because of this. Such a class would also behave slightly differently as the apply method constructor would not take an outerPointer. This is because the companion object already has an outer pointer and that will be used when creating the inner class object. Maybe it would be possible to add support for such cases but it would require more changes and is probably out of scope for this PR.

But just to be clear both the test and the code was wrong before this PR and they were wrong in such a way were they cancelled out. And the new spec in ExpressionEncoderSuite in this PR that tests at a "higher level" shows also that the previous code was wrong as that test case will fail on master.

[eejbyfeldt]

@HyukjinKwon Does my comment make sense? Would be great if this could make into the 3.3 branch before 3.3.1-rc2.

[HyukjinKwon]

Looks making sense to me.

[HyukjinKwon]

Merged to master and branch-3.3.

[MaxGekk]

@eejbyfeldt @HyukjinKwon Can we consider the error:

java.lang.RuntimeException: Couldn't find a valid constructor on ...

as an internal one. Could you take a look at the PR, please: #45302