HyukjinKwon commented on code in PR #59184:
URL: https://github.com/apache/spark/pull/59184#discussion_r4155783249
##########
sql/core/src/test/resources/sql-tests/analyzer-results/join-asof-containers.sql.out:
##########
@@ -352,6 +354,6 @@ BEGIN
FETCH cur INTO fetched_time, fetched_bid;
CLOSE cur;
SELECT fetched_time, fetched_bid;
-END
+END;
Review Comment:
**Non-blocking (P2):** This hunk is unrelated to the UDF refactor and
doesn't look like a regeneration result. `inputs/join-asof-containers.sql` is
LF and ends the block with `END;`, and `SQLQueryTestHelper` strips the trailing
`;` from `--QUERY-DELIMITER` blocks (`tempStr.stripSuffix(";")`). The generated
query text is therefore `...END`, matching
`results/join-asof-containers.sql.out` and the previous version of this file.
With `END;` here, the `SQL query did not match` assertion in SQLQueryTestSuite
fails for this query in analyzer mode. Could you regenerate this golden on an
LF checkout, which should drop this hunk, and revert the `--IMPORT` trim and
`\r\n` normalization changes in `SQLQueryTestHelper`? They seem to be local
CRLF workarounds outside this PR's scope.
**Verification:**
- **Regression:** The SQL golden-file suite passes on an LF checkout for
join-asof-containers.sql in analyzer mode.
- **Inspection:** The diff for SQLQueryTestHelper relative to the merge
target is empty.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/CreateUserDefinedFunctionCommand.scala:
##########
@@ -81,6 +86,49 @@ object CreateUserDefinedFunctionCommand {
}
}
+ // scalastyle:off argcount
+ def apply(
+ name: FunctionIdentifier,
+ inputParamText: Option[String],
+ returnTypeText: String,
+ exprText: Option[String],
+ queryText: Option[String],
+ comment: Option[String],
+ collation: Option[String],
+ isDeterministic: Option[Boolean],
+ containsSQL: Option[Boolean],
+ language: RoutineLanguage,
+ isTableFunc: Boolean,
+ isTemp: Boolean,
+ ignoreIfExists: Boolean,
+ replace: Boolean
+ ): CreateUserDefinedFunctionCommand = {
+ // scalastyle:on argcount
+ val child = if (isTemp) {
+ ResolvedIdentifier(
+ FakeSystemCatalog,
+ Identifier.of(Array(CatalogManager.SESSION_NAMESPACE), name.funcName))
+ } else {
Review Comment:
**Nit (P3):** Nothing calls this overload. Both parser call sites pass the
LogicalPlan child, and the package is internal. As written it also drops
`name.catalog` for persistent functions, so a future caller passing `cat.db.f`
would resolve `db.f` against the current catalog instead. I'd remove it.
Relatedly, `CreateSQLFunctionCommand.name` only runs after analysis, so its
`UnresolvedIdentifier` branch, which keeps only head/second-to-last/last parts,
could just throw an internal error instead of rebuilding a lossy identifier.
**Verification:**
- **Compile:** The sql/core main and test sources compile without the
removed overload.
- **Behavior:** Existing temp and persistent SQL UDF creation tests still
pass.
##########
project/MimaExcludes.scala:
##########
@@ -46,7 +46,30 @@ object MimaExcludes {
"org.apache.spark.ml.regression.DecisionTreeRegressionModel.numLeave"),
// [SPARK-59154] Remove unused prediction variance helper after inlining
its implementation.
ProblemFilters.exclude[DirectMissingMethodProblem](
-
"org.apache.spark.ml.regression.DecisionTreeRegressionModel.predictVariance")
+
"org.apache.spark.ml.regression.DecisionTreeRegressionModel.predictVariance"),
+ // [SPARK-50698][SQL] Refactor CreateUserDefinedFunction command to extend
UnaryRunnableCommand
Review Comment:
**Nit (P3):** These entries aren't needed. `defaultExcludes` already has
`ProblemFilters.exclude[Problem]("org.apache.spark.sql.catalyst.*")` and
`("org.apache.spark.sql.execution.*")` (around lines 193-194), and every
version's list inherits it. Could you drop the 11 added filters so the curated
per-release list stays limited to real API changes?
**Verification:**
- **Compatibility:** The MiMa check still passes after the entries are
removed.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -700,27 +700,13 @@ class ResolveSessionCatalog(val catalogManager:
CatalogManager)
case CreateFunction(ResolvedIdentifier(catalog, _), _, _, _, _) =>
throw
QueryCompilationErrors.missingCatalogCreateFunctionAbilityError(catalog)
- case c @ CreateUserDefinedFunction(
- CreateFunctionInSessionCatalog(ident), _, _, _, _, _, _, _, _, _, _,
_, _) =>
- CreateUserDefinedFunctionCommand(
- FunctionIdentifier(ident.table, ident.database, ident.catalog),
- c.inputParamText,
- c.returnTypeText,
- c.exprText,
- c.queryText,
- c.comment,
- c.collation,
- c.isDeterministic,
- c.containsSQL,
- c.language,
- c.isTableFunc,
- isTemp = false,
- c.ignoreIfExists,
- c.replace)
-
- case CreateUserDefinedFunction(
- ResolvedIdentifier(catalog, _), _, _, _, _, _, _, _, _, _, _, _, _) =>
- throw
QueryCompilationErrors.missingCatalogCreateFunctionAbilityError(catalog)
+ case c: CreateUserDefinedFunctionCommand if !c.isTemp =>
+ c.child match {
+ case CreateFunctionInSessionCatalog(_) => c
+ case ResolvedIdentifier(catalog, _) =>
Review Comment:
**Nit (P3):** This guard is the only thing that stops a SQL UDF from being
created through a non-session catalog, and it's now a rewritten match with a
`case _ => c` fallthrough. Could you add a test, for example next to `CREATE
FUNCTION: only support session catalog` in DataSourceV2FunctionSuite, that runs
`CREATE FUNCTION testcat.ns.f() RETURNS INT RETURN 1` and checks
`MISSING_CATALOG_ABILITY.CREATE_FUNCTION`? The existing test only covers the
`AS 'class'` form, which goes through `CreateFunction`.
**Verification:**
- **Regression:** The new test asserts the error condition and catalog
parameter for SQL UDF creation in a v2 test catalog.
--
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]
