Repository navigation
[SPARK-54682][SQL] Support showing parameters in DESCRIBE PROCEDURE - #53437
allisonwang-db wants to merge 4 commits into
Conversation
| val procedureCatalog = catalog.asProcedureCatalog | ||
| case UnresolvedProcedure(CatalogAndIdentifier(catalog, ident)) => | ||
| if (!catalog.isInstanceOf[ProcedureCatalog]) { | ||
| throw QueryCompilationErrors.missingCatalogProceduresAbilityError(catalog) |
There was a problem hiding this comment.
does catalog.asProcedureCatalog do this check?
| // which differs from the StructType used by internal stored procedures (handled by | ||
| // formatParameters). | ||
| private def formatProcedureParameters(params: Array[ProcedureParameter]): Seq[String] = { | ||
| val modes = tabulate(params.map(_.mode().toString).toSeq) |
There was a problem hiding this comment.
The impl here is really awkward, can we iterate param just once?
params.map { param =>
s"${param.mode} ${param.name} ..."
}
| // UnboundProcedure requires binding to retrieve parameters. We try to bind with an empty | ||
| // argument list to get the parameters. If the procedure requires arguments, binding might | ||
| // fail. In that case, we suppress the exception and just show the procedure metadata | ||
| // without parameters. |
There was a problem hiding this comment.
I'm afraid this is a common case, not rare
There was a problem hiding this comment.
It could be common for overloaded procedures or procedures that strictly validate inputs. For example, if a procedure behaves differently based on input types (like sum(int, int) vs sum(string, string)), it might not be able to report a single static list of parameters without knowing the arguments.
This fallback make sure DESCRIBE at least shows the procedure's existence and description instead of crashing when this happens.
There was a problem hiding this comment.
It seems it will work for the new Iceberg procedures (those procedures dont require arguments to bind)
I agree the comment sounds a bit confusing. its not about the procedure 'requiring arguments' (it sounds like its talking about at runtime), its more about whether it can bind succeesfully without arguments?
Also , i am wondering , is the expectation that we do not show anything for the procedures that need arguments for binding?
There was a problem hiding this comment.
I understand it's a best effort approach, but I can imagine if we do it in the current way, we will get many questions from users - why DESCRIBE does not show parameters for some procedures.
for functions, it relies on a description to provide the args info. I prefer this way given the situation.
There was a problem hiding this comment.
I think the purpose of DESCRIBE PROCEDURE is to provide a quick way for users to check how to use the procedure. Without parameters, its utility is severely limited, providing only the name and description.
While I agree it can be inconsistent, I think from a usability perspective we should provide parameter information to users whenever we can. I've updated the PR to use SimpleProcedure added by @cloud-fan.
There was a problem hiding this comment.
maybe later we can add some more methods to non simple UnboundProcedure , to better hint what arguments it supports? + @aokolnychyi as well
eed3d03 to
cb680b8
Compare
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
I think its a good improvement
| // UnboundProcedure requires binding to retrieve parameters. We try to bind with an empty | ||
| // argument list to get the parameters. If the procedure requires arguments, binding might | ||
| // fail. In that case, we suppress the exception and just show the procedure metadata | ||
| // without parameters. |
There was a problem hiding this comment.
It seems it will work for the new Iceberg procedures (those procedures dont require arguments to bind)
I agree the comment sounds a bit confusing. its not about the procedure 'requiring arguments' (it sounds like its talking about at runtime), its more about whether it can bind succeesfully without arguments?
Also , i am wondering , is the expectation that we do not show anything for the procedures that need arguments for binding?
| append(buffer, "Description:", procedure.description()) | ||
|
|
||
| try { | ||
| val bound = procedure.bind(new StructType()) |
There was a problem hiding this comment.
We can support SimpleProcedure only, added in #53595
cb680b8 to
20c721c
Compare
|
thanks, merging to master! |
4 participants
What changes were proposed in this pull request?
This PR updates the DESCRIBE PROCEDURE command to correctly resolve V2 procedures and display detailed parameter information.
Previously, DESCRIBE PROCEDURE did not provide parameter details. This change enhances the command to:
Why are the changes needed?
Users need to know the parameter signatures of stored procedures to call them correctly. Without this change, DESCRIBE PROCEDURE provided insufficient information for using a procedure. This change also makes the command more like DESCRIBE FUNCTION.
Does this PR introduce any user-facing change?
Yes, DESCRIBE PROCEDURE output now includes a "Parameters" section listing all arguments with their types and defaults.
How was this patch tested?
Existing tests.
Was this patch authored or co-authored using generative AI tooling?
Yes