HyukjinKwon commented on code in PR #57079:
URL: https://github.com/apache/spark/pull/57079#discussion_r3549855444
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -177,6 +182,13 @@ object RaiseErrorExpressionBuilder extends
ExpressionBuilder {
*/
@ExpressionDescription(
usage = "_FUNC_(expr [, message]) - Throws an exception if `expr` is not
true.",
+ arguments = """
+ Arguments:
+ * expr - The expression that is expected to be true.
+ An expression that evaluates to a string.
Review Comment:
`assert_true(expr [, message])` throws if `expr` is not true, so `expr` is
the boolean condition — it desugars to `If(left, Literal(null),
RaiseError(right))` (misc.scala:199), where `left` is `If`'s predicate.
```suggestion
An expression that evaluates to a boolean.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -177,6 +182,13 @@ object RaiseErrorExpressionBuilder extends
ExpressionBuilder {
*/
@ExpressionDescription(
usage = "_FUNC_(expr [, message]) - Throws an exception if `expr` is not
true.",
+ arguments = """
+ Arguments:
+ * expr - The expression that is expected to be true.
+ An expression that evaluates to a string.
+ * message - The message for the exception thrown when the expression is
not true.
+ An expression that evaluates to a map.
Review Comment:
`message` is the exception text (passed to `RaiseError`), a string — not a
map. `raise_error` above documents its message correctly as "a string".
```suggestion
An expression that evaluates to a string.
```
##########
python/pyspark/sql/functions/builtin.py:
##########
@@ -14397,14 +14735,17 @@ def assert_true(col: "ColumnOrName", errMsg:
Optional[Union[Column, str]] = None
Parameters
----------
col : :class:`~pyspark.sql.Column` or column name
- column name or column that represents the input column to test
+ column name or column that represents the input column to test.
+ A column that evaluates to a string.
Review Comment:
`col` is the boolean condition tested ("Returns `null` if the input column
is `true`"), not a string.
```suggestion
A column that evaluates to a boolean.
```
##########
python/pyspark/sql/functions/builtin.py:
##########
@@ -14397,14 +14735,17 @@ def assert_true(col: "ColumnOrName", errMsg:
Optional[Union[Column, str]] = None
Parameters
----------
col : :class:`~pyspark.sql.Column` or column name
- column name or column that represents the input column to test
+ column name or column that represents the input column to test.
+ A column that evaluates to a string.
errMsg : :class:`~pyspark.sql.Column` or literal string, optional
- A Python string literal or column containing the error message
+ A Python string literal or column containing the error message.
+ A column that evaluates to a map.
Review Comment:
`errMsg` is the error message string, not a map.
```suggestion
A column that evaluates to a string.
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -10212,6 +10608,8 @@ object functions {
*
* @group array_funcs
* @since 2.4.0
+ * @return
+ * Returns a column of the same type as the input.
Review Comment:
`array_min` returns the array's **element** type, not the array type —
`ArrayMin.dataType = ArrayType(dt,_) => dt`. "Same type as the input" is only
correct for functions whose output equals their input type (`abs`, `reverse`,
`round`, the `try_*` arithmetic, `nvl`, etc.), which are fine as-is.
Same mislabel on the other six collection-element functions here (all say
"same type as the input" but return the element/value type): `array_max`
(10623), `element_at` (9351), `try_element_at` (9366), `get` (9378), `explode`
(9824), `explode_outer` (9836) — and their PySpark mirrors. Suggest e.g.
"Returns a column of the element type of the input array." (and for
`element_at`/`try_element_at`, "...or the value type of the input map."; for
`explode`, that it produces rows of the element type, or key/value columns for
a map). This is the same issue @dongjoon-hyun flagged on `decode`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala:
##########
@@ -308,6 +328,12 @@ trait CeilFloorExpressionBuilderBase extends
ExpressionBuilder {
// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_(expr[, scale]) - Returns the smallest number after rounding
up that is not smaller than `expr`. An optional `scale` parameter can be
specified to control the rounding behavior.",
+ arguments = """
+ Arguments:
+ * expr - The expression to round up.
+ An expression that evaluates to a double, decimal, or long.
+ * scale - The number of decimal places to round to.
Review Comment:
`scale` has no type and no constant marker, but
`CeilFloorExpressionBuilderBase.build` requires `scale.foldable &&
scale.dataType == IntegerType`. The peer `round`/`bround` document the
identical `d` argument fully (mathExpressions.scala:1885).
```suggestion
* scale - The number of decimal places to round to.
An expression that evaluates to an integer. Must be a constant.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -2872,6 +2981,13 @@ case class ElementAt(
_FUNC_(map, key) - Returns value for given key. The function always
returns NULL
if the key is not contained in the map.
""",
+ arguments = """
+ Arguments:
+ * array - The array or map to retrieve an element from.
+ An expression that evaluates to an array.
Review Comment:
The description line right above says "array or map", and `ElementAt`
accepts `MapType` — so this drops the map case. For a map, `index` is the key
(any type), not an integer. Compare `size`, which the PR documents as "an
expression that evaluates to an array or map".
```suggestion
An expression that evaluates to an array or map.
```
##########
python/pyspark/sql/functions/builtin.py:
##########
@@ -19653,14 +20234,17 @@ def element_at(col: "ColumnOrName", extraction: Any)
-> Column:
Parameters
----------
col : :class:`~pyspark.sql.Column` or str
- name of column containing array or map
+ name of column containing array or map.
+ A column that evaluates to an array.
Review Comment:
Same as the Scala side: the prose above says "array or map", and
`element_at` accepts a map, so this should not be array-only. `try_element_at`
and `get` have the same array-only param line. (Minor, non-blocking: PySpark
`try_element_at` has no `Returns` block while `element_at` does.)
```suggestion
A column that evaluates to an array or map.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala:
##########
@@ -564,6 +617,12 @@ case class Floor(child: Expression) extends
UnaryMathExpression(math.floor, "FLO
// scalastyle:off line.size.limit
@ExpressionDescription(
usage = " _FUNC_(expr[, scale]) - Returns the largest number after rounding
down that is not greater than `expr`. An optional `scale` parameter can be
specified to control the rounding behavior.",
+ arguments = """
+ Arguments:
+ * expr - The expression to round down.
+ An expression that evaluates to a double, decimal, or long.
+ * scale - The number of decimal places to round to.
Review Comment:
Same as `ceil`: `floor`'s `scale` must be a constant integer
(`CeilFloorExpressionBuilderBase.build`), but the type and constant marker are
missing.
```suggestion
* scale - The number of decimal places to round to.
An expression that evaluates to an integer. Must be a constant.
```
##########
python/pyspark/sql/functions/builtin.py:
##########
@@ -22650,11 +23284,13 @@ def array_min(col: "ColumnOrName") -> Column:
----------
col : :class:`~pyspark.sql.Column` or str
The name of the column or an expression that represents the array.
+ A column that evaluates to an array.
Returns
-------
:class:`~pyspark.sql.Column`
A new column that contains the minimum value of each array.
+ Returns a column of the same type as the input.
Review Comment:
Same as the Scala side: `array_min` returns the element type, not the array
type. This "same type as the input" line is also wrong on `array_max`,
`element_at`, `get`, `explode`, `explode_outer` in this file (all return the
element/value type of a collection input).
--
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]
