MaxGekk commented on code in PR #56902:
URL: https://github.com/apache/spark/pull/56902#discussion_r3504521135
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SQLFunction.scala:
##########
@@ -299,6 +300,27 @@ object SQLFunction {
parser.parseExpression(text)
}
+ /**
+ * Pads `arguments` with the trailing parameters' declared defaults so the
result aligns
+ * one-to-one with `param`'s fields, parsing each default and casting it to
the parameter type.
+ * A missing parameter without a default raises
[[QueryCompilationErrors.wrongNumArgsError]].
+ */
+ def padArgumentsWithDefaults(
+ arguments: Seq[Expression],
+ param: StructType,
+ functionName: String,
+ parser: ParserInterface): Seq[Expression] = {
+ arguments ++ param.takeRight(param.size - arguments.size).map { field =>
+ field.getDefault() match {
+ case Some(default) =>
+ Cast(parseDefault(default, parser), field.dataType)
Review Comment:
The scalar extraction itself is behavior-preserving (this is byte-identical
to the old inline in `makeSQLFunctionPlan`). But the shipped result diverges
from the PR description on the two points the refactor is *for*:
1. **Only the scalar site delegates.** `padArgumentsWithDefaults` is called
only from `makeSQLFunctionPlan`; `makeSQLTableFunctionPlan` still inlines its
own near-identical padding (untyped — `parseDefault(...)` with no `Cast`),
unchanged. So "have both delegate to it" isn't realized and the table-path
duplication remains.
2. **The helper casts; nothing is dropped.** The description says it
"returns the parsed defaults untyped … so the scalar path drops a redundant
Cast," but this line casts (`Cast(parseDefault(...), field.dataType)`) exactly
as the old scalar code did — the scalar path is unchanged. (Your "What changes"
section — "casting it to the parameter type" — does match the code; it's the
summary paragraph that doesn't.)
Because the helper casts while the table site's padding is untyped, the
table path can't delegate to it as-is without gaining a `Cast` (redundant given
the downstream `Alias(Cast(outer, param.dataType))`, but still a change). Was
the table migration / cast-drop meant to be in this PR (i.e. make the shared
helper untyped and have both delegate), or is this intentionally scalar-only
for the single-pass analyzer and the summary paragraph is stale? Either
finishing the extraction or trimming the description to match would resolve it.
--
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]
