HyukjinKwon commented on code in PR #57079:
URL: https://github.com/apache/spark/pull/57079#discussion_r3555438612
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala:
##########
@@ -59,6 +59,15 @@ import org.apache.spark.unsafe.types.UTF8String
`k` An optional INTEGER literal greater than 0. If k is not specified,
it defaults to 5.
`maxItemsTracked` An optional INTEGER literal greater than or equal to k
and has upper limit of 1000000. If maxItemsTracked is not specified, it
defaults to 10000.
""",
+ arguments = """
+ Arguments:
+ * expr - The expression to compute the top k most frequent items of.
+ An expression of any type.
Review Comment:
`expr` isn't actually "any type": `checkInputDataTypes` routes it through
`ApproxTopK.isDataTypeSupported` (line 237-244), which accepts only
boolean/byte/short/int/long/float/double/date/timestamp/timestamp_ntz/string/decimal
— binary is explicitly excluded ("ItemsSketch seems cannot count the frequency
correctly") and array/map/struct/interval/variant are omitted, so e.g.
`approx_top_k(binary_col)` fails with `"binary columns are not supported"`.
Unlike `mode`/`first`/`any_value` (genuine `AnyDataType`), this warrants the
accepted atomic set.
```suggestion
An expression that evaluates to a boolean, numeric, date, timestamp,
or string.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala:
##########
@@ -488,6 +497,13 @@ object ApproxTopKAggregateBuffer {
_FUNC_(expr, maxItemsTracked) - Accumulates items into a sketch.
`maxItemsTracked` An optional positive INTEGER literal with upper limit
of 1000000. If maxItemsTracked is not specified, it defaults to 10000.
""",
+ arguments = """
+ Arguments:
+ * expr - The expression whose values are accumulated into the sketch.
+ An expression of any type.
Review Comment:
Same as `approx_top_k` above: `approx_top_k_accumulate`'s `expr` is gated by
`isDataTypeSupported`, not truly "any type".
```suggestion
An expression that evaluates to a boolean, numeric, date, timestamp,
or string.
```
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -3314,12 +3354,15 @@ object functions {
*
* @group math_funcs
* @since 1.3.0
+ * @return
+ * Returns a column of the same type as the input.
*/
def abs(e: Column): Column = Column.fn("abs", e)
/**
* @return
- * inverse cosine of `e` in radians, as if computed by
`java.lang.Math.acos`
+ * inverse cosine of `e` in radians, as if computed by
`java.lang.Math.acos` Returns a column
Review Comment:
The appended "Returns a column..." sentence has no separating period, so
Scaladoc renders it as a run-on: "...as if computed by `java.lang.Math.acos`
Returns a column that evaluates to a double." This is the same defect
@dongjoon-hyun flagged on PySpark `builtin.py:700` (`dividend`) — fixed there
in this revision, but the Scala `@return` blocks weren't swept. It affects ~50
`@return` blocks here (acos/acosh/asin/atan/atan2/cos/cot/sec/sin/tan family,
`width_bucket`, `levenshtein`:5460, and the ~30 date-part extractors), while
the `@param` appended sentences are all correctly punctuated. Add a period
before "Returns" in each:
```suggestion
* inverse cosine of `e` in radians, as if computed by
`java.lang.Math.acos`. Returns a column
```
--
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]
