[SPARK-58359][SQL] Require the arguments field in built-in function documentation

[HyukjinKwon]

What changes were proposed in this pull request?

This PR makes the arguments field of @ExpressionDescription a required part of built-in function documentation, alongside the already-required usage, examples, since, and group fields.

• ExpressionInfoSuite (the SPARK-32870 test) now asserts info.getArguments.nonEmpty for every registered function, except functions that take no arguments, which are enumerated in a new noArgumentsSet (e.g. pi, current_date, current_timestamp, input_file_name, uuid, spark_partition_id). The existing format assertions (the value must start with \n Arguments:\n and end with \n ) continue to apply.
• Backfills arguments documentation for 118 built-in expressions across 45 files that were previously missing it.

Note on scope: the enforced list is derived from what is actually registered in the expression FunctionRegistry. Expressions whose ExpressionInfo is produced through a builder object are validated via their builder class; some of these already document arguments (e.g. minute/second) and were not part of the backfill, while others whose builders were missing it (e.g. max_by/min_by) were backfilled on the builder class. Expressions that live only in the table function registry are likewise validated via their builder class and were not part of the backfill. This change does not affect the generated sql-expression-schema.md golden file.

Why are the changes needed?

Most built-in functions already document their arguments, but it was not enforced, so newly added expressions could omit it and the generated function docs would be inconsistent. Requiring the field keeps the built-in function documentation complete and uniform, and turns a missing Arguments: section into a test failure rather than a silent gap.

Does this PR introduce any user-facing change?

Yes. The generated documentation for the affected built-in functions now includes an Arguments: section describing each argument. This is a documentation-only change; there are no behavior changes.

How was this patch tested?

Existing ExpressionInfoSuite covers this, extended to assert that arguments is present for all registered functions (minus the zero-argument exemptions).

build/sbt 'sql/testOnly org.apache.spark.sql.expressions.ExpressionInfoSuite'

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

This pull request and its description were written by Isaac.

[HyukjinKwon]

cc @cloud-fan

[HyukjinKwon]

One minor doc-accuracy note on the PR description itself (code looks correct):

The description says builders like max_by/min_by "already document arguments" and "were not part of the backfill." But this PR does add arguments to MaxByBuilder and MinByBuilder in MaxMinByK.scala (they were missing it, and the new assert(info.getArguments.nonEmpty) would have failed for them otherwise). The minute/second half of that sentence is accurate — those weren't touched.

Suggest dropping max_by/min_by from that sentence, or moving them into the backfilled list, so the stated scope matches the diff.

Otherwise LGTM: the enforcement mirrors the existing SPARK-32870 pattern (noArgumentsSet alongside ignoreSet, fail-closed), the 118 arguments = additions match the claim, all changes are annotation-string only (no behavior change), and the noArgumentsSet exemptions are all genuinely zero-arg. I verified the builder-registered functions are actually reached by the test via FunctionRegistry.expressionBuilder.

[HyukjinKwon]

Addressed in the PR description: the scope note now separates the two builder cases — minute/second already documented arguments (not backfilled), while max_by/min_by were missing it and were backfilled on their builder class. No code change needed.

[uros-b]

I left just one comment - please address, otherwise looks good thank you @HyukjinKwon!

[cloud-fan]

0 blocking, 1 non-blocking, 0 nits.
The enforcement approach is sound, but one newly added argument description overstates the accepted input type.

Correctness (1)

• sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/kllAggregates.scala:214: kll_sketch_agg_float accepts only FloatType (inputTypes at line 258), so documenting expr as accepting a double is inaccurate. Please describe this input as float-only; users who pass a DOUBLE otherwise receive a type-check error despite the new documentation. -- see inline

Verification

I traced the new registry-wide assertion and checked the promoted documentation-contract candidate against KllSketchAggFloat.inputTypes. That implementation accepts FloatType only (plus an optional integer k), while the new documentation says expr may be a float or double.

[dongjoon-hyun]

Nice cleanup — enforcing arguments alongside the existing required fields closes a long-standing documentation gap, and keeping the format assertions applied after the noArgumentsSet exemption is a clean structure. I verified the backfilled descriptions against the implementations and the only CI failure is the unrelated Oracle docker infra flake (ORA-12516). A few minor documentation-accuracy points:

1. array() / struct() / named_struct(): the new docs say "one or more" expressions, but zero arguments are valid (SELECT array() returns an empty array). "Zero or more" would be accurate.
2. least / greatest: these require at least two arguments (wrongNumArgsError when children.length <= 1), which the new docs don't mention.
3. min vs max: max says "any orderable type ... NULL values are ignored" while min only says "any comparable type" without the NULL note. Since they are symmetric, the same wording would be better.
4. reflect vs try_reflect: reflect correctly says "static method" but try_reflect just says "method to invoke"; both call static methods.
5. KLL k parameter: kll_sketch_agg_* / kll_merge_agg_* docs omit the Optional marker and the default (200 for the agg variants), although the usage text says it is optional and other entries in this PR follow the "Optional. ... Defaults to ..." pattern.

None of these are blockers — the fact-level ones are 1 and 2. LGTM once those are addressed.

[cloud-fan]

1 addressed, 0 remaining, 2 new. (0 newly introduced, 2 late catches, 0 previously raised.)
0 blocking, 2 non-blocking, 0 nits.
The enforcement is sound, but two newly added argument descriptions understate the functions' supported or required arity.

Correctness (2)

• sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:60: array() and struct() both accept zero arguments, but the new argument descriptions say that one or more expressions are required. Change those two descriptions to zero or more so the generated function documentation covers the supported empty-value forms. -- see inline
• sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:1325: least and greatest reject calls with fewer than two arguments, but the new argument documentation does not state that minimum. Document that at least two expressions are required so users do not infer that the variadic form accepts a single value. -- see inline

Verification

I traced the new registry assertion and its zero-argument exemptions, checked the documentation additions against the relevant expression implementations, and re-adjudicated the prior KLL concern against the current float-only wording. CreateArray and CreateStruct.create accept empty child sequences, while Least and Greatest explicitly reject one or fewer children.

[cloud-fan]

2 addressed, 0 remaining, 0 new.
0 blocking, 0 non-blocking, 0 nits.
The current head addresses both findings from the prior review, and I found no remaining correctness, design, performance, or documentation-quality issues.

Verification

I re-adjudicated both prior findings against the current head: array and struct now document zero-or-more arguments, and least and greatest now state their two-argument minimum. I also checked the registry-wide exemption/assertion structure and reviewed the contract, text-quality, and local-efficiency scanner results across the complete changed-file inventory. No test command was run as part of this review.

[HyukjinKwon]

Merge Summary:

• merged into master 5c78674
• merged into branch-4.x 017d530
• merged into branch-4.3 be6ce34

Posted by merge_spark_pr.py