You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "TeodorDjelic (via GitHub)" <gi...@apache.org> on 2025/07/29 13:47:44 UTC
[PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
TeodorDjelic opened a new pull request, #51705:
URL: https://github.com/apache/spark/pull/51705
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251605567
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1681,15 +1681,15 @@ case class TableSpec(
}
/**
- * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
+ * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLES command.
Review Comment:
I wouldn't do it. I think this need for plurals in keywords is a bit over-the-top.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251792735
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -721,7 +721,7 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case c: CreateVariable
if c.resolved && c.defaultExpr.child.containsPattern(PLAN_EXPRESSION) =>
- val ident = c.name.asInstanceOf[ResolvedIdentifier]
+ val ident = c.names(0).asInstanceOf[ResolvedIdentifier]
Review Comment:
AFAIK, this is only for error highlighting when the error is concerning the DECLARE expression as a whole.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2258844383
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
This error might be confusing as the conflicting variable will not be created at the end, so this "already exists" variable does not really exist. Shall we simply call `create` one by one?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2244902646
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6375,32 +6375,36 @@ class AstBuilder extends DataTypeAstBuilder
}
/**
- * Create a [[CreateVariable]] command.
+ * Create a [[CreateVariables]] command.
*
* For example:
* {{{
- * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name
+ * DECLARE [OR REPLACE] [COMMA SEPARATED VARIABLES] [db_name.]variable_name
* [dataType] [defaultExpression];
* }}}
*
* We will add CREATE VARIABLE for persisted variable definitions to this, hence the name.
*/
- override def visitCreateVariable(ctx: CreateVariableContext): LogicalPlan = withOrigin(ctx) {
+ override def visitCreateVariables(ctx: CreateVariablesContext): LogicalPlan = withOrigin(ctx) {
val dataTypeOpt = Option(ctx.dataType()).map(typedVisit[DataType])
val defaultExpression = if (ctx.variableDefaultExpression() == null) {
if (dataTypeOpt.isEmpty) {
throw new ParseException(
errorClass = "INVALID_SQL_SYNTAX.VARIABLE_TYPE_OR_DEFAULT_REQUIRED",
messageParameters = Map.empty,
- ctx.identifierReference)
+ ctx.identifierReferences.get(0))
}
DefaultValueExpression(Literal(null, dataTypeOpt.get), "null")
} else {
val default = visitVariableDefaultExpression(ctx.variableDefaultExpression())
dataTypeOpt.map { dt => default.copy(child = Cast(default.child, dt)) }.getOrElse(default)
}
- CreateVariable(
- withIdentClause(ctx.identifierReference(), UnresolvedIdentifier(_)),
+ CreateVariables(
+ ctx.identifierReferences.asScala.map (
+ ctx => {
Review Comment:
nit: `ctx` naming is confusing here. Maybe something like `identifierReference` ?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/V2CommandStrategy.scala:
##########
@@ -27,8 +27,9 @@ object V2CommandStrategy extends Strategy {
// TODO: move v2 commands to here which are not data source v2 related.
override def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
- case CreateVariable(ident: ResolvedIdentifier, defaultExpr, replace) =>
- CreateVariableExec(ident, defaultExpr, replace) :: Nil
+ case CreateVariables(idents: IndexedSeq[LogicalPlan], defaultExpr, replace)
+ if idents.forall(_.isInstanceOf[ResolvedIdentifier]) =>
Review Comment:
```suggestion
case CreateVariables(idents: IndexedSeq[ResolvedIdentifier], defaultExpr, replace) =>
```
Would this work? If so it would be cleaner, if not the current approach is fine
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala:
##########
@@ -2743,6 +2744,66 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(sqlScript, expected = expected)
}
+ test("local variable - multiple variables declared at once") {
Review Comment:
What happens if we try to declare multiple variables with the same name in the same DECLARE ? We should have a test case for this
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
+ case c @ CreateVariables(plans, _, _) =>
// From scripts we can only create local variables, which must be unqualified,
// and must not be DECLARE OR REPLACE.
- val resolved = if (withinSqlScript) {
- if (c.replace) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.REPLACE_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts))
- )
- }
-
- if (nameParts.length != 1) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.QUALIFIED_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts)))
- }
-
- SqlScriptingContextManager.get().map(_.getVariableManager)
- .getOrElse(throw SparkException.internalError(
- "Scripting local variable manager should be present in SQL script."))
- .qualify(nameParts.last)
- } else {
- val resolvedIdentifier = catalogManager.tempVariableManager.qualify(nameParts.last)
-
- assertValidSessionVariableNameParts(nameParts, resolvedIdentifier)
- resolvedIdentifier
+ val resolved = plans.map {
+ case plan: UnresolvedIdentifier =>
+ val unresolvedIdentifier = plan
Review Comment:
why not `case unresolvedIdentifier: UnresolvedIdentifier`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6375,32 +6375,36 @@ class AstBuilder extends DataTypeAstBuilder
}
/**
- * Create a [[CreateVariable]] command.
+ * Create a [[CreateVariables]] command.
*
* For example:
* {{{
- * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name
+ * DECLARE [OR REPLACE] [COMMA SEPARATED VARIABLES] [db_name.]variable_name
Review Comment:
`[VARIABLE]` in this comment referers to the optional VARIABLE keyword which can be included in DECLARE. The `[db_name.]variable_name` part of the comment is the one which should be updated.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251833808
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -721,7 +721,7 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case c: CreateVariable
if c.resolved && c.defaultExpr.child.containsPattern(PLAN_EXPRESSION) =>
- val ident = c.name.asInstanceOf[ResolvedIdentifier]
+ val ident = c.names(0).asInstanceOf[ResolvedIdentifier]
Review Comment:
This was discussed offline with @davidm-db @dusantism-db and we agreed this approach is fine because declaration of the first variable failed, so we print out the message for the first variable, and the rest of variables are not even processed after that (we don't have to change the error message then).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245817910
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1681,15 +1681,15 @@ case class TableSpec(
}
/**
- * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
+ * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLES command.
*/
-case class CreateVariable(
- name: LogicalPlan,
+case class CreateVariables(
+ names: IndexedSeq[LogicalPlan],
Review Comment:
No reason, I'll change it to Seq since other parts of code also use Seq
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257111131
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,20 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends Command with SupportsSubquery {
+ override def children: Seq[LogicalPlan] = names
+ override def mapChildren(f: LogicalPlan => LogicalPlan): LogicalPlan = {
Review Comment:
what's wrong with the default implementation?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245817910
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1681,15 +1681,15 @@ case class TableSpec(
}
/**
- * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
+ * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLES command.
*/
-case class CreateVariable(
- name: LogicalPlan,
+case class CreateVariables(
+ names: IndexedSeq[LogicalPlan],
Review Comment:
No reason, I'll change it to Seq since other parts of code also use Seq
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245908117
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala:
##########
@@ -2743,6 +2744,66 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(sqlScript, expected = expected)
}
+ test("local variable - multiple variables declared at once") {
Review Comment:
A guard against that needs to be added.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2273907017
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariableExec.scala:
##########
@@ -43,25 +46,57 @@ case class CreateVariableExec(
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ val variableTuples = resolvedIdentifiers.map(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+
+ (normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(), varDef)
+ })
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // create local variables if we are in a script, otherwise create session variable
+ val variableManager =
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifiers.head.catalog == FakeLocalCatalog)
// If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
// will always be present.
.getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+
+ if (!replace) {
+ val uniqueNames = mutable.Set[String]()
+
+ variableTuples.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+
+ if (uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "DUPLICATE_DECLARE_VARIABLE",
Review Comment:
shall we report this error even if `replace` is false?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2260357295
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
1. We are making this work for Session Variables as well? If not, we should. If yes, then your statement doesn't hold.
2. Variables are created left to right, but considering `DECLARE x; DECLARE x;` vs `DECLARE x, x;`, in the first case we would have variable `x` created and then second declare would fail, but the `x` would persist, whereas in the second case variable `x` is not created at all. Thus, having a little bit different message to differentiate the new case we are introducing would be nice.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51705:
URL: https://github.com/apache/spark/pull/51705#issuecomment-3191618273
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2256974629
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -1369,6 +1369,39 @@ trait QuaternaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
protected def withNewChildrenInternal(newFirst: T, newSecond: T, newThird: T, newFourth: T): T
}
+trait NaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
Review Comment:
Agreed, Nary classes were removed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251677669
##########
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala:
##########
@@ -1173,12 +1173,12 @@ class ForStatementExec(
private def createDeclareVarExec(varName: String): SingleStatementExec = {
val defaultExpression = DefaultValueExpression(
Literal(null, queryColumnNameToDataType(varName)), "null")
- val declareVariable = CreateVariable(
- UnresolvedIdentifier(Seq(varName)),
+ val declareVariables = CreateVariable(
Review Comment:
nit: unnecessary rename
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257173102
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
Review Comment:
We could inline the code. My only dilemma is that the single variable version `VariableManager::create` is also used inside one place outside the tests, so maybe it would be useful to leave the multiple variable version of `VariableManager::create`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2248242803
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/V2CommandStrategy.scala:
##########
@@ -27,8 +27,9 @@ object V2CommandStrategy extends Strategy {
// TODO: move v2 commands to here which are not data source v2 related.
override def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
- case CreateVariable(ident: ResolvedIdentifier, defaultExpr, replace) =>
- CreateVariableExec(ident, defaultExpr, replace) :: Nil
+ case CreateVariables(idents: IndexedSeq[LogicalPlan], defaultExpr, replace)
+ if idents.forall(_.isInstanceOf[ResolvedIdentifier]) =>
Review Comment:
It seems that scala does not support enforcing template type restriction in runtime, and an if guard is needed. It does not even compile because of that without if.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2244956475
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -719,9 +719,9 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case alter: AlterTableCommand =>
checkAlterTableCommand(alter)
- case c: CreateVariable
+ case c: CreateVariables
Review Comment:
This rename is not necessary but introduces a lot of diff in this PR. For reference `SetVariable` also supports setting multiple variables
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2256720644
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,12 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends NaryCommand with SupportsSubquery {
Review Comment:
I'll rewrite the code with just `Command` and without `NaryCommand` and `NaryLike`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255789108
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
Review Comment:
it's only called by one place?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255780972
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6379,7 +6379,7 @@ class AstBuilder extends DataTypeAstBuilder
*
* For example:
* {{{
- * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name
+ * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name [COMMA [db_name.]variable_name]*
Review Comment:
```suggestion
* DECLARE [OR REPLACE] [VARIABLE] variable_name [COMMA variable_name]*
```
A qualified variable name can have many name parts. Let's just use `variable_name`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6379,7 +6379,7 @@ class AstBuilder extends DataTypeAstBuilder
*
* For example:
* {{{
- * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name
+ * DECLARE [OR REPLACE] [VARIABLE] [db_name.]variable_name [COMMA [db_name.]variable_name]*
Review Comment:
```suggestion
* DECLARE [OR REPLACE] [VARIABLE] variable_name [COMMA variable_name]*
```
A qualified variable name can have many name parts. Let's just use `variable_name` an identifier.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2256684804
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
Review Comment:
Yes, only from CreateVariableExec::run.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2260522430
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
Agreed with introducing a new error message. We can keep `VARIABLE_ALREADY_EXISTS` if a variable with the same name existed before we called this statement, and have a different error for the new case.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2256716394
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -1369,6 +1369,39 @@ trait QuaternaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
protected def withNewChildrenInternal(newFirst: T, newSecond: T, newThird: T, newFourth: T): T
}
+trait NaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
Review Comment:
how is it different from a logical plan with a `children: Seq[LogicalPlan]` field?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245507446
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1681,15 +1681,15 @@ case class TableSpec(
}
/**
- * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
+ * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLES command.
Review Comment:
@srielau do we want to change grammar in any way for session variable declarations - i.e. do we want to do support `DECLARE VARS/VARIABLES` as well or just keep `DECLARE VAR/VARIABLE`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
+ case c @ CreateVariables(plans, _, _) =>
// From scripts we can only create local variables, which must be unqualified,
// and must not be DECLARE OR REPLACE.
Review Comment:
I think we should move this comment a bit below, above the corresponding piece of code.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariablesExec.scala:
##########
@@ -23,45 +23,51 @@ import org.apache.spark.sql.catalyst.{InternalRow, SqlScriptingContextManager}
import org.apache.spark.sql.catalyst.analysis.{FakeLocalCatalog, ResolvedIdentifier}
import org.apache.spark.sql.catalyst.catalog.VariableDefinition
import org.apache.spark.sql.catalyst.expressions.{Attribute, ExpressionsEvaluator, Literal}
-import org.apache.spark.sql.catalyst.plans.logical.DefaultValueExpression
+import org.apache.spark.sql.catalyst.plans.logical.{DefaultValueExpression, LogicalPlan}
import org.apache.spark.sql.connector.catalog.Identifier
import org.apache.spark.sql.execution.datasources.v2.LeafV2CommandExec
/**
* Physical plan node for creating a variable.
Review Comment:
```suggestion
* Physical plan node for creating variables.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1681,15 +1681,15 @@ case class TableSpec(
}
/**
- * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
+ * The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLES command.
*/
-case class CreateVariable(
- name: LogicalPlan,
+case class CreateVariables(
+ names: IndexedSeq[LogicalPlan],
Review Comment:
any reason to use `IndexedSeq` instead of simple `Seq`? Either I'm missing something, or I don't see any place in code where indexing is strictly required.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariablesExec.scala:
##########
@@ -23,45 +23,51 @@ import org.apache.spark.sql.catalyst.{InternalRow, SqlScriptingContextManager}
import org.apache.spark.sql.catalyst.analysis.{FakeLocalCatalog, ResolvedIdentifier}
import org.apache.spark.sql.catalyst.catalog.VariableDefinition
import org.apache.spark.sql.catalyst.expressions.{Attribute, ExpressionsEvaluator, Literal}
-import org.apache.spark.sql.catalyst.plans.logical.DefaultValueExpression
+import org.apache.spark.sql.catalyst.plans.logical.{DefaultValueExpression, LogicalPlan}
import org.apache.spark.sql.connector.catalog.Identifier
import org.apache.spark.sql.execution.datasources.v2.LeafV2CommandExec
/**
* Physical plan node for creating a variable.
*/
-case class CreateVariableExec(
- resolvedIdentifier: ResolvedIdentifier,
+case class CreateVariablesExec(
+ logicalPlansOfResolvedIdentifiers: IndexedSeq[LogicalPlan],
defaultExpr: DefaultValueExpression,
replace: Boolean) extends LeafV2CommandExec with ExpressionsEvaluator {
override protected def run(): Seq[InternalRow] = {
+
+ val resolvedIdentifiers =
+ logicalPlansOfResolvedIdentifiers.map(_.asInstanceOf[ResolvedIdentifier])
+
val scriptingVariableManager = SqlScriptingContextManager.get().map(_.getVariableManager)
val tempVariableManager = session.sessionState.catalogManager.tempVariableManager
val exprs = prepareExpressions(Seq(defaultExpr.child), subExprEliminationEnabled = false)
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ resolvedIdentifiers.foreach(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
- // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
- // will always be present.
- .getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+ // create local variable if we are in a script, otherwise create session variable
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
+ // will always be present.
+ .getOrElse(tempVariableManager)
+ .create(
+ normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
+ varDef,
+ replace)
+ })
Review Comment:
Suggested rewrite - I'd move everything to a `VariableManager`, i.e. support passing `Seq` of variables there. In the trait, we can have one `public def create` and one `protected def create` - public one would do the checks (i.e. check for uniqueness in the same declare statement + use `get` to check if the variable is already declared), whereas protected one would be used/called from the public one to actually declare variable.
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala:
##########
@@ -2536,6 +2536,7 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
| SELECT localVar;
| SELECT session.localVar;
| END;
+ | DECLARE x, y, z, w = 2;
Review Comment:
Is this a leftover? It doesn't seem related to this test, so I wouldn't add it here.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -719,9 +719,9 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case alter: AlterTableCommand =>
checkAlterTableCommand(alter)
- case c: CreateVariable
+ case c: CreateVariables
Review Comment:
Agreed with this! We can keep `CreateVariable` in this PR definitely, to simplify the changes. If we want/there's a need to rename, we can do it in a follow-up PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
+ case c @ CreateVariables(plans, _, _) =>
// From scripts we can only create local variables, which must be unqualified,
// and must not be DECLARE OR REPLACE.
Review Comment:
Also, maybe add a new comment explaining the new logic a bit - i.e. we are resolving only `UnresolvedIdentifiers` so they don't get resolved by another rules, everything else is not touched so the other roles can create `UnresolvedIdentifiers` out of the other plans.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255787024
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -1369,6 +1369,39 @@ trait QuaternaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
protected def withNewChildrenInternal(newFirst: T, newSecond: T, newThird: T, newFourth: T): T
}
+trait NaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
Review Comment:
what's the benefit of this trait?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257109916
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,20 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends Command with SupportsSubquery {
+ override def children: Seq[LogicalPlan] = names
+ override def mapChildren(f: LogicalPlan => LogicalPlan): LogicalPlan = {
Review Comment:
why do we need to overwrite this?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2260804620
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariablesExec.scala:
##########
@@ -23,45 +23,51 @@ import org.apache.spark.sql.catalyst.{InternalRow, SqlScriptingContextManager}
import org.apache.spark.sql.catalyst.analysis.{FakeLocalCatalog, ResolvedIdentifier}
import org.apache.spark.sql.catalyst.catalog.VariableDefinition
import org.apache.spark.sql.catalyst.expressions.{Attribute, ExpressionsEvaluator, Literal}
-import org.apache.spark.sql.catalyst.plans.logical.DefaultValueExpression
+import org.apache.spark.sql.catalyst.plans.logical.{DefaultValueExpression, LogicalPlan}
import org.apache.spark.sql.connector.catalog.Identifier
import org.apache.spark.sql.execution.datasources.v2.LeafV2CommandExec
/**
* Physical plan node for creating a variable.
*/
-case class CreateVariableExec(
- resolvedIdentifier: ResolvedIdentifier,
+case class CreateVariablesExec(
+ logicalPlansOfResolvedIdentifiers: IndexedSeq[LogicalPlan],
defaultExpr: DefaultValueExpression,
replace: Boolean) extends LeafV2CommandExec with ExpressionsEvaluator {
override protected def run(): Seq[InternalRow] = {
+
+ val resolvedIdentifiers =
+ logicalPlansOfResolvedIdentifiers.map(_.asInstanceOf[ResolvedIdentifier])
+
val scriptingVariableManager = SqlScriptingContextManager.get().map(_.getVariableManager)
val tempVariableManager = session.sessionState.catalogManager.tempVariableManager
val exprs = prepareExpressions(Seq(defaultExpr.child), subExprEliminationEnabled = false)
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ resolvedIdentifiers.foreach(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
- // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
- // will always be present.
- .getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+ // create local variable if we are in a script, otherwise create session variable
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
+ // will always be present.
+ .getOrElse(tempVariableManager)
+ .create(
+ normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
+ varDef,
+ replace)
+ })
Review Comment:
@srielau Do we want to make this new multivariable `DECLARE` atomic, so if one variable declaration fails, should they all fail?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2260242014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
If we wanted to change the error, I don’t think a new `VARIABLE_ALREADY_EXISTS` sub-category would be sufficient, because the error concerns the uniqueness of variables within the same `DECLARE` statement, which haven't been fully created yet. However, I’d still say that `VARIABLE_ALREADY_EXISTS` is appropriate, since from the user’s perspective, variables are declared left to right.
Moreover, if `VARIABLE_ALREADY_EXISTS` is acceptable for declaring the same variable in two separate DECLARE statements, then it should be equally valid for declaring the same variable within a single DECLARE statement. After all, these are local variables, and if the script fails, they don’t escape the script’s scope — the script crashes regardless.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
If we wanted to change the error, I don’t think a new `VARIABLE_ALREADY_EXISTS` sub-category would be sufficient, because the error concerns the uniqueness of variables within the same `DECLARE` statement, which haven't been fully created yet. However, I’d still say that `VARIABLE_ALREADY_EXISTS` is appropriate, since from the user’s perspective, variables are declared left to right.
Moreover, if `VARIABLE_ALREADY_EXISTS` is acceptable for declaring the same variable in two separate DECLARE statements, then it should be equally valid for declaring the same variable within a single `DECLARE` statement. After all, these are local variables, and if the script fails, they don’t escape the script’s scope — the script crashes regardless.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245468530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
+ case c @ CreateVariables(plans, _, _) =>
// From scripts we can only create local variables, which must be unqualified,
// and must not be DECLARE OR REPLACE.
Review Comment:
I think we should move this comment a bit below, above the corresponding piece of code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245773621
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala:
##########
@@ -2536,6 +2536,7 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
| SELECT localVar;
| SELECT session.localVar;
| END;
+ | DECLARE x, y, z, w = 2;
Review Comment:
It is, I'll remove 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2274089041
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariableExec.scala:
##########
@@ -43,25 +46,57 @@ case class CreateVariableExec(
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ val variableTuples = resolvedIdentifiers.map(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+
+ (normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(), varDef)
+ })
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // create local variables if we are in a script, otherwise create session variable
+ val variableManager =
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifiers.head.catalog == FakeLocalCatalog)
// If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
// will always be present.
.getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+
+ if (!replace) {
+ val uniqueNames = mutable.Set[String]()
+
+ variableTuples.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+
+ if (uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "DUPLICATE_DECLARE_VARIABLE",
Review Comment:
It would be more consistent to report the error either way
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2274121279
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -1428,6 +1428,12 @@
],
"sqlState" : "42734"
},
+ "DUPLICATE_DECLARE_VARIABLE" : {
Review Comment:
I don't think this name for the error condition is the best we can come up with - it sounds like DECLARE is duplicated, it doesn't point to the underlying issue in any easily conclusive way.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2259629476
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
The main purpose of this PR is to avoid having to call `create` one by one (which is basically what you would do today with multiple separate `DECLARE` statements) and provide potential for more efficient code. This way, we avoid multiple query executions.
Regarding the error, it still points to the proper place in code where the error is and I'd hope that this is not super hard to figure out. But, I don't think it would hurt to create a sub-category of `VARIABLE_ALREADY_EXISTS` for this specific case.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255788211
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,12 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends NaryCommand with SupportsSubquery {
Review Comment:
I don't see how `NaryCommand` is better than just `command`. We need to implement a `withChildren` method anyway.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251838034
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6392,15 +6392,19 @@ class AstBuilder extends DataTypeAstBuilder
throw new ParseException(
errorClass = "INVALID_SQL_SYNTAX.VARIABLE_TYPE_OR_DEFAULT_REQUIRED",
messageParameters = Map.empty,
- ctx.identifierReference)
+ ctx.identifierReferences.get(0))
Review Comment:
Same reason as the first comment.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6392,15 +6392,19 @@ class AstBuilder extends DataTypeAstBuilder
throw new ParseException(
errorClass = "INVALID_SQL_SYNTAX.VARIABLE_TYPE_OR_DEFAULT_REQUIRED",
messageParameters = Map.empty,
- ctx.identifierReference)
+ ctx.identifierReferences.get(0))
Review Comment:
Same reason as the first comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251781486
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6392,15 +6392,19 @@ class AstBuilder extends DataTypeAstBuilder
throw new ParseException(
errorClass = "INVALID_SQL_SYNTAX.VARIABLE_TYPE_OR_DEFAULT_REQUIRED",
messageParameters = Map.empty,
- ctx.identifierReference)
+ ctx.identifierReferences.get(0))
Review Comment:
Maybe I am missing some discussion about this, but we mention only first variable name here?
Is this intentional @TeodorDjelic, @davidm-db?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2256670498
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -1369,6 +1369,39 @@ trait QuaternaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
protected def withNewChildrenInternal(newFirst: T, newSecond: T, newThird: T, newFourth: T): T
}
+trait NaryLike[T <: TreeNode[T]] { self: TreeNode[T] =>
Review Comment:
The benefit of this trait is to have a variable number of logical plan children, so we can reuse the existing variable resolving logical plans.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2259616448
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
Review Comment:
case sensitivity is handled in exec nodes - that's how it was done for session variables so we didn't change it.
cc @dusantism-db if I'm missing anything.
+1 on adding tests for case-sensitivity if they are missing, I think we already have them for single variable case, but should cover this as well.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2258840896
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
Review Comment:
do we need to consider case sensitivity? do we have tests for 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51705: [SPARK-52998][Core] Multiple variables inside declare
URL: https://github.com/apache/spark/pull/51705
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2274116439
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariableExec.scala:
##########
@@ -43,25 +46,57 @@ case class CreateVariableExec(
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ val variableTuples = resolvedIdentifiers.map(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+
+ (normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(), varDef)
+ })
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // create local variables if we are in a script, otherwise create session variable
+ val variableManager =
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifiers.head.catalog == FakeLocalCatalog)
// If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
// will always be present.
.getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+
+ if (!replace) {
+ val uniqueNames = mutable.Set[String]()
+
+ variableTuples.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+
+ if (uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "DUPLICATE_DECLARE_VARIABLE",
Review Comment:
please add a bit of comments explaining what each error is for and why are we checking for duplicates in both cases. looks like it might be a bit confusing for someone reading the code for the first time
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257107784
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
Review Comment:
are we going to call it in more places? if not let's inline the code in `CreateVariableExec::run`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257584311
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,20 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends Command with SupportsSubquery {
+ override def children: Seq[LogicalPlan] = names
+ override def mapChildren(f: LogicalPlan => LogicalPlan): LogicalPlan = {
Review Comment:
Default implementation works fine actually, I removed the override
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255781627
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
- // From scripts we can only create local variables, which must be unqualified,
- // and must not be DECLARE OR REPLACE.
- val resolved = if (withinSqlScript) {
- if (c.replace) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.REPLACE_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts))
- )
- }
-
- if (nameParts.length != 1) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.QUALIFIED_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts)))
- }
-
- SqlScriptingContextManager.get().map(_.getVariableManager)
- .getOrElse(throw SparkException.internalError(
- "Scripting local variable manager should be present in SQL script."))
- .qualify(nameParts.last)
- } else {
- val resolvedIdentifier = catalogManager.tempVariableManager.qualify(nameParts.last)
-
- assertValidSessionVariableNameParts(nameParts, resolvedIdentifier)
- resolvedIdentifier
+ case c @ CreateVariable(plans, _, _) =>
Review Comment:
```suggestion
case c @ CreateVariable(identifiers, _, _) =>
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2257574180
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -1684,12 +1684,20 @@ case class TableSpec(
* The logical plan of the DECLARE [OR REPLACE] TEMPORARY VARIABLE command.
*/
case class CreateVariable(
- name: LogicalPlan,
+ names: Seq[LogicalPlan],
defaultExpr: DefaultValueExpression,
- replace: Boolean) extends UnaryCommand with SupportsSubquery {
- override def child: LogicalPlan = name
- override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
- copy(name = newChild)
+ replace: Boolean) extends Command with SupportsSubquery {
+ override def children: Seq[LogicalPlan] = names
+ override def mapChildren(f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val newNames = names.map(f)
+ val allUnchanged = names.iterator.zip(newNames.iterator).forall { case (a, b) => a eq b }
+ if (allUnchanged) this
+ else copy(names = newNames)
+ }
Review Comment:
nit: add empty line after }
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251839410
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -6392,15 +6392,19 @@ class AstBuilder extends DataTypeAstBuilder
throw new ParseException(
errorClass = "INVALID_SQL_SYNTAX.VARIABLE_TYPE_OR_DEFAULT_REQUIRED",
messageParameters = Map.empty,
- ctx.identifierReference)
+ ctx.identifierReferences.get(0))
Review Comment:
Same reason as the first comment. This was discussed offline with @davidm-db @dusantism-db and we agreed this approach is fine because declaration of the first variable failed, so we print out the message for the first variable, and the rest of variables are not even processed after that (we don't have to change the error message then).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2260242014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
If we wanted to change the error, I don’t think a new `VARIABLE_ALREADY_EXISTS` sub-category would be sufficient, because the error concerns the uniqueness of variables within the same `DECLARE` statement, which haven't been fully created yet. However, I’d still say that `VARIABLE_ALREADY_EXISTS` is appropriate, since from the user’s perspective, variables are declared left to right.
Moreover, if `VARIABLE_ALREADY_EXISTS` is acceptable for declaring the same variable in two separate DECLARE statements, then it should be equally valid for declaring the same variable within a single `DECLARE` statement. After all, these are local variables, and if the script fails, they don’t escape the script’s scope; the script crashes regardless.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2276391649
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
Review Comment:
We handle case sensitivity during analysis, so it is already handled when we get to execution, which is when this method is called.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2245468530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
+ case c @ CreateVariables(plans, _, _) =>
// From scripts we can only create local variables, which must be unqualified,
// and must not be DECLARE OR REPLACE.
Review Comment:
I think we should move this comment a bit below, above the corresponding piece of code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2244931689
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariablesExec.scala:
##########
@@ -23,45 +23,51 @@ import org.apache.spark.sql.catalyst.{InternalRow, SqlScriptingContextManager}
import org.apache.spark.sql.catalyst.analysis.{FakeLocalCatalog, ResolvedIdentifier}
import org.apache.spark.sql.catalyst.catalog.VariableDefinition
import org.apache.spark.sql.catalyst.expressions.{Attribute, ExpressionsEvaluator, Literal}
-import org.apache.spark.sql.catalyst.plans.logical.DefaultValueExpression
+import org.apache.spark.sql.catalyst.plans.logical.{DefaultValueExpression, LogicalPlan}
import org.apache.spark.sql.connector.catalog.Identifier
import org.apache.spark.sql.execution.datasources.v2.LeafV2CommandExec
/**
* Physical plan node for creating a variable.
*/
-case class CreateVariableExec(
- resolvedIdentifier: ResolvedIdentifier,
+case class CreateVariablesExec(
+ logicalPlansOfResolvedIdentifiers: IndexedSeq[LogicalPlan],
defaultExpr: DefaultValueExpression,
replace: Boolean) extends LeafV2CommandExec with ExpressionsEvaluator {
override protected def run(): Seq[InternalRow] = {
+
+ val resolvedIdentifiers =
+ logicalPlansOfResolvedIdentifiers.map(_.asInstanceOf[ResolvedIdentifier])
+
val scriptingVariableManager = SqlScriptingContextManager.get().map(_.getVariableManager)
val tempVariableManager = session.sessionState.catalogManager.tempVariableManager
val exprs = prepareExpressions(Seq(defaultExpr.child), subExprEliminationEnabled = false)
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+ resolvedIdentifiers.foreach(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
- // create local variable if we are in a script, otherwise create session variable
- scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
- // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
- // will always be present.
- .getOrElse(tempVariableManager)
- .create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+ // create local variable if we are in a script, otherwise create session variable
+ scriptingVariableManager
+ .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ // If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
+ // will always be present.
+ .getOrElse(tempVariableManager)
+ .create(
+ normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
+ varDef,
+ replace)
+ })
Review Comment:
I don't think this is right, since `variableManager.create` can throw (both scripting and temp) - e.g. if the variable already exists. So in the example
`DECLARE foo, bar, baz INT;
`
If `bar` already exists, `foo` would be created but `baz` would not, as the exception would be thrown when we get to `bar`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251669067
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNodeSuite.scala:
##########
@@ -177,7 +177,7 @@ class SqlScriptingExecutionNodeSuite extends SparkFunSuite with SharedSparkSessi
case forStmt: TestForStatement => forStmt.label.get
case _: NoOpStatementExec => "NOOP"
case createStmt: SingleStatementExec
- if createStmt.parsedPlan.isInstanceOf[CreateVariable] => "CreateVariable"
Review Comment:
Let's undo this change as well
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/v2/CreateVariableExec.scala:
##########
@@ -43,25 +43,25 @@ case class CreateVariableExec(
initializeExprs(exprs, 0)
val initValue = Literal(exprs.head.eval(), defaultExpr.dataType)
- val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
- resolvedIdentifier.identifier
- } else {
- Identifier.of(
- resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
- resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
- }
- val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
-
- // create local variable if we are in a script, otherwise create session variable
+ // create local variables if we are in a script, otherwise create session variable
scriptingVariableManager
- .filter(_ => resolvedIdentifier.catalog == FakeLocalCatalog)
+ .filter(_ => resolvedIdentifiers.head.catalog == FakeLocalCatalog)
// If resolvedIdentifier.catalog is FakeLocalCatalog, scriptingVariableManager
// will always be present.
.getOrElse(tempVariableManager)
.create(
- normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(),
- varDef,
- replace)
+ resolvedIdentifiers.map(resolvedIdentifier => {
+ val normalizedIdentifier = if (session.sessionState.conf.caseSensitiveAnalysis) {
+ resolvedIdentifier.identifier
+ } else {
+ Identifier.of(
+ resolvedIdentifier.identifier.namespace().map(_.toLowerCase(Locale.ROOT)),
+ resolvedIdentifier.identifier.name().toLowerCase(Locale.ROOT))
+ }
+ val varDef = VariableDefinition(normalizedIdentifier, defaultExpr.originalSQL, initValue)
+
+ (normalizedIdentifier.namespace().toSeq :+ normalizedIdentifier.name(), varDef)
Review Comment:
nit: I think it would be more readable to do this map before the method call and save it to a val
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -105,6 +145,9 @@ class TempVariableManager extends VariableManager with DataTypeErrorsBase {
@GuardedBy("this")
private val variables = new mutable.HashMap[String, VariableDefinition]
+ protected def getVariableNameForError(variableName: String): String =
Review Comment:
```suggestion
override def getVariableNameForError(variableName: String): String =
```
Shouldn't it be this, unless I'm missing something?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -105,6 +145,9 @@ class TempVariableManager extends VariableManager with DataTypeErrorsBase {
@GuardedBy("this")
private val variables = new mutable.HashMap[String, VariableDefinition]
+ protected def getVariableNameForError(variableName: String): String =
Review Comment:
Same for scripting variable manager
##########
sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala:
##########
@@ -2743,6 +2743,85 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(sqlScript, expected = expected)
}
+ test("local variable - multiple variables declared at once") {
Review Comment:
We should add some similar tests for non scripting variables. Take a look at `sql-session-variables.sql`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,46 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created
Review Comment:
nit: could you expand this comment to explain the type `Seq[(Seq[String], VariableDefinition)]` . It's not immediately obvious what is it's structure
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251698558
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,46 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return
Review Comment:
Please fill out the `@return` part of the comment
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2255782664
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -40,35 +40,40 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsDown {
// We only support temp variables for now and the system catalog is not properly implemented
// yet. We need to resolve `UnresolvedIdentifier` for variable commands specially.
- case c @ CreateVariable(UnresolvedIdentifier(nameParts, _), _, _) =>
- // From scripts we can only create local variables, which must be unqualified,
- // and must not be DECLARE OR REPLACE.
- val resolved = if (withinSqlScript) {
- if (c.replace) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.REPLACE_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts))
- )
- }
-
- if (nameParts.length != 1) {
- throw new AnalysisException(
- "INVALID_VARIABLE_DECLARATION.QUALIFIED_LOCAL_VARIABLE",
- Map("varName" -> toSQLId(nameParts)))
- }
-
- SqlScriptingContextManager.get().map(_.getVariableManager)
- .getOrElse(throw SparkException.internalError(
- "Scripting local variable manager should be present in SQL script."))
- .qualify(nameParts.last)
- } else {
- val resolvedIdentifier = catalogManager.tempVariableManager.qualify(nameParts.last)
-
- assertValidSessionVariableNameParts(nameParts, resolvedIdentifier)
- resolvedIdentifier
+ case c @ CreateVariable(plans, _, _) =>
+ // We resolve only UnresolvedIdentifiers, and pass on the other nodes
+ val resolved = plans.map {
+ case unresolvedIdentifier: UnresolvedIdentifier =>
Review Comment:
```suggestion
case UnresolvedIdentifier(nameParts, _) =>
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "TeodorDjelic (via GitHub)" <gi...@apache.org>.
TeodorDjelic commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251792735
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -721,7 +721,7 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case c: CreateVariable
if c.resolved && c.defaultExpr.child.containsPattern(PLAN_EXPRESSION) =>
- val ident = c.name.asInstanceOf[ResolvedIdentifier]
+ val ident = c.names(0).asInstanceOf[ResolvedIdentifier]
Review Comment:
AFAIK, this is only for error highlighting when the error is concerning the DECLARE expression as a whole.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2251766608
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -721,7 +721,7 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
case c: CreateVariable
if c.resolved && c.defaultExpr.child.containsPattern(PLAN_EXPRESSION) =>
- val ident = c.name.asInstanceOf[ResolvedIdentifier]
+ val ident = c.names(0).asInstanceOf[ResolvedIdentifier]
Review Comment:
Does this check only first variable?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2258847270
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
+ * @param variables Variables to be created,
+ * a list of tuples of nameParts and varDef for each variable
+ * @param overrideIfExists If true, the new variable will replace an existing one
+ * with the same identifier, if it exists.
+ */
+ final def create(
+ variables: Seq[(Seq[String], VariableDefinition)],
+ overrideIfExists: Boolean): Unit = synchronized {
+ if (!overrideIfExists) {
+ val uniqueNames = mutable.Set[String]()
+
+ variables.foreach(variable => {
+ val nameParts: Seq[String] = variable._1
+ val name = nameParts.last
+ if (get(nameParts).isDefined || uniqueNames.contains(name)) {
+ throw new AnalysisException(
+ errorClass = "VARIABLE_ALREADY_EXISTS",
Review Comment:
an alternative is we have a dedicated error for conflicting variable names in a single batch create request.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2258838802
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/VariableManager.scala:
##########
@@ -81,6 +81,47 @@ trait VariableManager {
* @return true if at least one variable exists, false otherwise.
*/
def isEmpty: Boolean
+
+ /**
+ *
+ * @param variableName Name of the variable
+ * @return variable name formatting for the error
+ */
+ protected def getVariableNameForError(variableName: String): String
+
+ /**
+ * Create a variable.
Review Comment:
```suggestion
* Create variables.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52998][Core] Multiple variables inside declare [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #51705:
URL: https://github.com/apache/spark/pull/51705#discussion_r2274121279
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -1428,6 +1428,12 @@
],
"sqlState" : "42734"
},
+ "DUPLICATE_DECLARE_VARIABLE" : {
Review Comment:
I don't think this name for the error condition is the best we can come up with - it sounds like DECLARE is duplicated, it doesn't point to the underlying issue in any way.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org