[SPARK-58086][SQL][DOCS] Document accepted input and return types in the Scala functions API#57184
[SPARK-58086][SQL][DOCS] Document accepted input and return types in the Scala functions API#57184HyukjinKwon wants to merge 1 commit into
Conversation
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Could you make the CI happy?
The scalafmt check failed on sql/connect or sql/connect at following occurrences:
org.apache.maven.plugin.MojoExecutionException: Scalafmt: Unformatted files found
Error: Failed to execute goal org.antipathy:mvn-scalafmt_2.13:1.1.1713302731.c3d0074:format (default-cli) on project spark-sql-api_2.13: Error formatting Scala files: Scalafmt: Unformatted files found -> [Help 1]
Before submitting your change, please make sure to format your code using the following command:
./build/mvn scalafmt:format -Dscalafmt.skip=false -Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl sql/api -pl sql/connect/common -pl sql/connect/server -pl sql/connect/shims -pl sql/connect/client/jvm
df0490f to
f9b4f14
Compare
…the Scala functions API ### What changes were proposed in this pull request? This documents, for every built-in function in the Scala `functions` API (`functions.scala`), the accepted input type of each argument (`@param`) and the return type of each DataFrame function (`@return`), using the numeric umbrella where numeric subtypes cast in and `timestamp` for TIMESTAMP_NTZ in type phrases. Subtask of SPARK-57999, split out from apache#57079 for reviewability. This PR covers only the Scala `functions.scala` surface. ### Why are the changes needed? The Scala `functions` Scaladoc previously had no per-argument type information and no documented return type for most functions. This fills that gap. ### Does this PR introduce _any_ user-facing change? Yes, documentation only. No behavioral or API change. ### How was this patch tested? Scala style/format (scalastyle + scalafmt) and the docs build pass. Types were derived from and verified against the analyzer's behavior and each expression's `inputTypes` / `dataType`. Co-authored-by: Isaac
f9b4f14 to
88a7746
Compare
MaxGekk
left a comment
There was a problem hiding this comment.
0 blocking, 3 non-blocking, 0 nits.
A careful, high-quality documentation pass — the type conventions hold up across the functions I sampled. One @return overstates its type; two smaller doc gaps.
Correctness (1)
- functions.scala:13749:
from_jsonnon-StructTypeoverloads are documented as returning a struct, but they also accept array/map schemas (verified: they returnArrayType/MapType) — see inline
Suggestions (1)
- functions.scala:13628:
posexplode/posexplode_outerhave a@parambut no@return, unlikeexplode/explode_outer— see inline
Nits (1)
- functions.scala:8619: comma splice + "of string" → "of strings" in the
search@param(pre-existing, but on a line this PR edits) — see inline
Verification
Confirmed finding #1 by running from_json against a local build: from_json(col, ArrayType(IntegerType)) → ArrayType(IntegerType,true), from_json(col, MapType(StringType,IntegerType)) → MapType(...), and from_json(col, lit("array<int>")) (Column overload) → ArrayType(...) — none a struct — while the StructType overload correctly returns a struct. Spot-checked seven other @returns (size→integer, array_contains→boolean, ceil→long, concat_ws→string, sqrt→double, abs→same-as-input, map_keys→array); all matched the docs.
| @@ -9548,6 +13745,8 @@ object functions { | |||
| * | |||
| * @group json_funcs | |||
| * @since 2.2.0 | |||
| * @return | |||
| * Returns a column that evaluates to a struct. | |||
There was a problem hiding this comment.
This @return ("evaluates to a struct") is accurate for the StructType overloads, but the overloads taking schema: DataType, schema: String, or schema: Column also accept ArrayType and MapType top-level schemas — the description just above even says "a MapType ... StructType or ArrayType with the specified schema", and JsonToStructs.dataType is the schema itself.
Verified against a local build:
from_json(col, ArrayType(IntegerType)) -> ArrayType(IntegerType,true)
from_json(col, MapType(StringType, IntegerType)) -> MapType(StringType,IntegerType,true)
from_json(col, lit("array<int>")) -> ArrayType(IntegerType,true)
from_json(col, StructType(...)) -> StructType(...) // the only struct case
So from_json(json, "array<int>") returns an array, not a struct. Consider, on the non-StructType overloads, something like:
| * Returns a column that evaluates to a struct. | |
| * Returns a column of the type given by the schema (a struct, array, or map). |
(Non-blocking. The StructType overloads' @return is correct as-is.)
| @@ -9453,6 +13624,8 @@ object functions { | |||
| * default column name `pos` for position, and `col` for elements in the array and `key` and | |||
| * `value` for elements in the map unless specified otherwise. | |||
| * | |||
| * @param e | |||
| * the target column to explode. A column that evaluates to an array or a map. | |||
There was a problem hiding this comment.
posexplode (and posexplode_outer just below) got a @param but no @return, whereas the parallel explode/explode_outer document one ("a column of the element type of the input array, or the key and value columns of the input map"). Worth mirroring explode's @return here, plus the added pos position column, so the generator family is documented consistently. (Non-blocking.)
| * @param search | ||
| * A column of string, If `search` is not found in `str`, `str` is returned unchanged. | ||
| * A column of string, If `search` is not found in `str`, `str` is returned unchanged. A |
There was a problem hiding this comment.
Minor: "A column of string, If search is not found ..." has a comma splice (the comma before "If" should be a period) and "of string" should be "of strings". This text is pre-existing, but since this PR edits the line (appends the type sentence), it's cheap to tidy in passing. (Nit.)
What changes were proposed in this pull request?
This documents, for every built-in function in the Scala
functionsAPI (functions.scala), theaccepted input type of each argument (
@param) and the return type of each DataFrame function(
@return), using the numeric umbrella where numeric subtypes cast in andtimestampforTIMESTAMP_NTZ in type phrases.
Subtask of SPARK-57999, split out from #57079 for reviewability. This PR covers only
the Scala
functions.scalasurface.Why are the changes needed?
The Scala
functionsScaladoc previously had no per-argument type information and no documentedreturn type for most functions. This fills that gap.
Does this PR introduce any user-facing change?
Yes, documentation only. No behavioral or API change.
How was this patch tested?
Scala style/format (scalastyle + scalafmt) and the docs build pass. Types were derived from and
verified against the analyzer's behavior and each expression's
inputTypes/dataType.Co-authored-by: Isaac