Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan closed pull request #47404: [SPARK-48338][SQL] Check variable declarations URL: https://github.com/apache/spark/pull/47404 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on PR #47404: URL: https://github.com/apache/spark/pull/47404#issuecomment-2247084382 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1689268400
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,42 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+val candidates = if (allowVarDeclare) {
+ compoundStatements.dropWhile {
+case SingleStatement(_: CreateVariable) => true
Review Comment:
This is just for my curiosity. If SQL scripting only allows creation of
local variable, then we can even fail earlier in the parser if the CREATE
VARIABLE uses more than one name part.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1689261990
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,42 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+val candidates = if (allowVarDeclare) {
+ compoundStatements.dropWhile {
+case SingleStatement(_: CreateVariable) => true
Review Comment:
Agreement with Serge is that we should be able to reference session
variables at all times using `system.session.`. Also, local variables
can be accessed by their name directly, but also using `.`
to allow access to all local variables in case some of them have been shadowed.
Though, what I explained is a resolution part not the creation. For the
creation, looking at the code and documentation, we should allow the same thing
as well:
https://github.com/user-attachments/assets/8e77f0f0-e954-4ead-894b-3ff59c3a48a5";>
I will check with Serge once again, just to confirm, but wouldn't block this
PR on it because we are preparing a POC for variables anyways which is supposed
to handle all local variable related stuff and we will resolve all such cases
there, if that's fine by you!
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1689241446
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,42 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+val candidates = if (allowVarDeclare) {
+ compoundStatements.dropWhile {
+case SingleStatement(_: CreateVariable) => true
Review Comment:
unrelated question: within SQL scripting, shall we only allow to create
single-part-name variables? e.g. should `CREATE VARIABLE system.session.name`
be disallowed?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
momcilomrk-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1688311354
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareVarStatement match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable]
+ .name.asInstanceOf[UnresolvedIdentifier]
+ .nameParts.last,
+parsedPlan.origin.line.get.toString)
+case _ =>
+ }
+
+} else {
+ val declareVarStatement = compoundStatements
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
Done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
momcilomrk-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1688310986
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
Done
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareVarStatement match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable]
+ .name.asInstanceOf[UnresolvedIdentifier]
+ .nameParts.last,
Review Comment:
Done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1687821095
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareVarStatement match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable]
+ .name.asInstanceOf[UnresolvedIdentifier]
+ .nameParts.last,
Review Comment:
We did it like this since we are still figuring out the best way to
implement local variables, and thought of addressing this "issue" then. But,
what you suggested seems like the thing that we will end up doing eventually,
so no harm to do it now 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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1687737158
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareVarStatement match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable]
+ .name.asInstanceOf[UnresolvedIdentifier]
+ .nameParts.last,
+parsedPlan.origin.line.get.toString)
+case _ =>
+ }
+
+} else {
+ val declareVarStatement = compoundStatements
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
A better way to avoid duplicated code
```
val candidates = if (allowVarDeclare) {
compoundStatements.dropWhile
} else {
compoundStatements
}
val varNames = candidates.filter...
...
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1687730964
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
nit:
```
val varNames = compoundStatements.dropWhile {
case SingleStatement(_: CreateVariable) => true
case _ => false
}.collectFirst {
case SingleStatement(c: CreateVariable) =>
c.name.asInstanceOf[UnresolvedIdentifier].nameParts
case _ => 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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1687734716
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareVarStatement match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable]
+ .name.asInstanceOf[UnresolvedIdentifier]
+ .nameParts.last,
Review Comment:
we should use the full variable name, not just the last name part. We can
use `DataTypeErrorsBase#toSQLId` to generate the name string
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
cloud-fan commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1687730964
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,48 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareVarStatement = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
nit:
```
compoundStatements.dropWhile {
case SingleStatement(_: CreateVariable) => true
case _ => false
}.find {
case SingleStatement(_: CreateVariable) => true
case _ => 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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1684289427
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,46 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareAfterPrefix = compoundStatements
+.dropWhile(statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declareAfterPrefix match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable].name.
+ asInstanceOf[UnresolvedIdentifier].nameParts.last,
+parsedPlan.origin.line.get.toString)
+case _ =>
+ }
+
+} else {
+ val declare = compoundStatements
+.filter(_.isInstanceOf[SingleStatement])
+
.find(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ declare match {
+case Some(SingleStatement(parsedPlan)) =>
+ throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning(
+parsedPlan.asInstanceOf[CreateVariable].name.
+ asInstanceOf[UnresolvedIdentifier].nameParts.last,
Review Comment:
when I split one chained method calls into multiple lines, I usually like to
start the line with the . because it's then clear at the first glance that it
is continuation from the row before... also, I try to group them logically.
so, I would rather do:
```
parsedPlan.asInstanceOf[CreateVariable]
.name.asInstanceOf[UnresolvedIdentifier]
.nameParts.last
```
same goes for the similar part of the code above.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1684286132
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,46 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowVarDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowVarDeclare) {
+ val declareAfterPrefix = compoundStatements
Review Comment:
we should probably remove "prefix" part from here as well.
let's use the same name here and in the `else` branch -
`declareVarStatement` for example.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
momcilomrk-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1684077096
##
common/utils/src/main/resources/error/error-conditions.json:
##
@@ -2941,6 +2941,23 @@
],
"sqlState" : "22029"
},
+ "INVALID_VARIABLE_DECLARATION": {
+"message" : [
+ "Invalid variable declaration."
Review Comment:
Added `varName` and `lineNumber` in subclass.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683095051
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,36 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowPrefixDeclare: Boolean): CompoundBody = {
Review Comment:
not sure if the `allowPrefixDeclare` is the proper name, it's unclear what
"prefix" means... maybe just `allowVarDeclare`?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683089130
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -161,11 +185,11 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
val labelText = beginLabelCtx.
map(_.multipartIdentifier().getText).getOrElse(java.util.UUID.randomUUID.toString).
toLowerCase(Locale.ROOT)
-visitCompoundBodyImpl(ctx.compoundBody(), Some(labelText))
+visitCompoundBodyImpl(ctx.compoundBody(), Some(labelText), true)
Review Comment:
we are usually using named parameters when passing explicit boolean values,
so it's clear what `true` means in this context. same goes for the `false`
below.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683087387
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,36 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowPrefixDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowPrefixDeclare) {
+ val declareAfterPrefix = compoundStatements.dropWhile(
+ statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.exists(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+
+ if(declareAfterPrefix) {
+throw SqlScriptingErrors.variableDeclarationOnlyAtBeginning()
+ }
+
+} else {
+ val declareExists = compoundStatements
+.filter(_.isInstanceOf[SingleStatement])
+
.exists(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
same here for `find` instead of `exists`
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683086403
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,36 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowPrefixDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowPrefixDeclare) {
+ val declareAfterPrefix = compoundStatements.dropWhile(
+ statement => statement.isInstanceOf[SingleStatement] &&
+
statement.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
+.filter(_.isInstanceOf[SingleStatement])
+
.exists(_.asInstanceOf[SingleStatement].parsedPlan.isInstanceOf[CreateVariable])
Review Comment:
once we add `varName` and `lineNumber` parameters to the error message,
you'll probably need to use `find` instead of `exists` to find the first wrong
variable declaration and construct error message based on 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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683084476
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##
@@ -133,12 +133,36 @@ class AstBuilder extends DataTypeAstBuilder with
SQLConfHelper with Logging {
private def visitCompoundBodyImpl(
ctx: CompoundBodyContext,
- label: Option[String]): CompoundBody = {
+ label: Option[String],
+ allowPrefixDeclare: Boolean): CompoundBody = {
val buff = ListBuffer[CompoundPlanStatement]()
ctx.compoundStatements.forEach(compoundStatement => {
buff += visit(compoundStatement).asInstanceOf[CompoundPlanStatement]
})
+val compoundStatements = buff.toList
+
+if (allowPrefixDeclare) {
+ val declareAfterPrefix = compoundStatements.dropWhile(
Review Comment:
nit: move `.dropWhile` part to the new line. it's how we usually do it, so
all functions are starting in a new row when there is a need to split into
multiple rows.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683079758
##
common/utils/src/main/resources/error/error-conditions.json:
##
@@ -2941,6 +2941,23 @@
],
"sqlState" : "22029"
},
+ "INVALID_VARIABLE_DECLARATION": {
+"message" : [
+ "Invalid variable declaration."
+],
+"subClass" : {
+ "NOT_ALLOWED_IN_SCOPE" : {
+"message": [
+ "Variable declaration is not allowed in this scope."
+]
+ },
+ "ONLY_AT_BEGINNING" : {
+"message": [
+ "Variable declaration is only possible at the beginning of the BEGIN
END compound."
Review Comment:
nit: you can remove BEGIN END part from the message, compound is literally
defined by BEGIN and END.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683078491
##
common/utils/src/main/resources/error/error-conditions.json:
##
@@ -2941,6 +2941,23 @@
],
"sqlState" : "22029"
},
+ "INVALID_VARIABLE_DECLARATION": {
+"message" : [
+ "Invalid variable declaration."
Review Comment:
note for other reviewers, we have agreed offline to add `varName` and
`lineNumber` parameters to the error message.
@momcilomrk-db Resolve this comment once it's 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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
davidm-db commented on code in PR #47404:
URL: https://github.com/apache/spark/pull/47404#discussion_r1683076936
##
common/utils/src/main/resources/error/error-conditions.json:
##
@@ -2941,6 +2941,23 @@
],
"sqlState" : "22029"
},
+ "INVALID_VARIABLE_DECLARATION": {
+"message" : [
+ "Invalid variable declaration."
+],
+"subClass" : {
+ "NOT_ALLOWED_IN_SCOPE" : {
+"message": [
+ "Variable declaration is not allowed in this scope."
+]
+ },
+ "ONLY_AT_BEGINNING" : {
+"message": [
+ "Variable declaration is only possible at the beginning of the BEGIN
END compound."
+]
+ }
+}
+ },
Review Comment:
missed to specify `sqlState` value at the end of
`INVALID_VARIABLE_DECLARATION` entry.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-48338][SQL] Check variable declarations [spark]
momcilomrk-db commented on PR #47404: URL: https://github.com/apache/spark/pull/47404#issuecomment-2236664403 @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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
