dongjoon-hyun commented on PR #57551: URL: https://github.com/apache/spark/pull/57551#issuecomment-5174989487
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. -- 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]
