Kontinuation commented on code in PR #704:
URL: https://github.com/apache/incubator-sedona/pull/704#discussion_r1007562119
##########
sql/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Constructors.scala:
##########
@@ -305,41 +325,29 @@ case class ST_GeomFromGeoJSON(inputExpressions:
Seq[Expression])
/**
* Return a Point from X and Y
*
- * @param inputExpressions This function takes 2 parameter which are point x
and y.
+ * @param inputExpressions This function takes 3 parameter which are point x,
y and z.
*/
case class ST_Point(inputExpressions: Seq[Expression])
- extends Expression with CodegenFallback with UserDataGeneratator {
- inputExpressions.betweenLength(2, 3)
Review Comment:
Assertions for expressions with optional parameters looks strange since all
parameters will receive their arguments before evaluation. This assertion
became unnecessary since the function builder registered to the catalog will
emit error messages for function calls with invalid number of parameters:
```
SELECT ST_Point(1)
>>> function ST_Point takes at least 2 argument(s), 1 argument(s) specified
```
For expressions with optional parameters like `ST_Point` we cannot simply
assert `inputExpressions.size == 3` since there's one weird behavior in the
implementation of the function builder: when 2 arguments were passed to
`ST_Point`, it will construct a `ST_Point` object using these 2 arguments
first, then call its `inputTypes` method to acknowledge that it actually has 3
parameters, so that it will construct the final `ST_Point` object with 2 user
specified arguments and one default argument.
There're [better
ways](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L110)
to support optional parameters without constructing intermediate invalid
expressions, however that may introduce huge changes, so I'd like to live with
it for now.
--
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]
