HyukjinKwon commented on a change in pull request #35378:
URL: https://github.com/apache/spark/pull/35378#discussion_r799119170
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
##########
@@ -254,6 +254,20 @@ case class StaticInvoke(
returnNullable: Boolean = true,
isDeterministic: Boolean = true) extends InvokeLike {
+ // This additional constructor is added to keep binary compatibility after
the addition of the
Review comment:
I do sympathize the pain. In order to address all such problems,
expressions for API (V2 expressions) are under heavy development as a long run
goal. I also agree that it's probably best to avoid the changes that
unnecessarily break the compatibility of private/internal API, e.g., if that
does not bring significant dev overhead.
For this PR, it would look awkward and confusing (see the comments in the
code): if the developers should keep the binary compatibility in the expression
at `StaticInvoke` and `Invoke` or all the expressions. In addition, we should
keep adding overloaded constructors, which is not ideal for private/internal
API.
`Encoder` and `Decoder` are indeed public but `ExpressionEncoder` is
currently not (that is under internal `catalyst` package). We do and guarantee,
with binary compatibility check, and maintain the binary compatibility and
backward compatibility [as
documented](https://spark.apache.org/versioning-policy.html) for public API but
not for internal API.
--
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]
