eejbyfeldt commented on code in PR #37837:
URL: https://github.com/apache/spark/pull/37837#discussion_r971579395
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala:
##########
@@ -423,7 +423,7 @@ class ObjectExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
inputTypes = Nil,
propagateNull = false,
dataType = ObjectType(classOf[ScroogeLikeExample]),
- outerPointer = Some(() => outerObj))
+ outerPointer = None)
Review Comment:
So normal inner classes (that uses outer pointer) are tested here in the
ExpressionEncoderSuite:
https://github.com/apache/spark/blob/ae08787f5c50e485ef4432a0c2da8b3b7290d725/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala#L229-L233
and here in the ObjectExpressionsSuite:
https://github.com/apache/spark/blob/ae08787f5c50e485ef4432a0c2da8b3b7290d725/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala#L408-L417
so such cases are covered.
Are you saying that we should add a test case for a class that only have an
apply side 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
