srielau commented on code in PR #49427:
URL: https://github.com/apache/spark/pull/49427#discussion_r1920370673
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -159,15 +159,104 @@ class AstBuilder extends DataTypeAstBuilder
script
}
+ private def assertSqlState(sqlState: String): Unit = {
+ val sqlStateRegex = "^[A-Za-z0-9]{5}$".r
+ if (sqlStateRegex.findFirstIn(sqlState).isEmpty
+ || sqlState.startsWith("00")
+ || sqlState.startsWith("01")
+ || sqlState.startsWith("XX")) {
+ throw SqlScriptingErrors.invalidSqlStateValue(CurrentOrigin.get,
sqlState)
+ }
+ }
+
+ override def visitConditionValue(ctx: ConditionValueContext): String = {
+ Option(ctx.sqlStateValue())
+ .map { sqlStateValueContext =>
+ val sqlState = sqlStateValueContext.getText.replace("'", "")
+ assertSqlState(sqlState)
+ sqlState
+ }
+ .getOrElse(ctx.getText)
+ }
+
+ override def visitConditionValues(ctx: ConditionValuesContext): Seq[String]
= {
+ val buff = scala.collection.mutable.Set[String]()
+ ctx.cvList.forEach { conditionValue =>
+ val elem = visit(conditionValue).asInstanceOf[String]
+ if (buff(elem)) {
+ throw
SqlScriptingErrors.duplicateConditionInHandlerDeclaration(CurrentOrigin.get,
elem)
+ }
+ buff += elem
+ }
+ buff.toSeq
+ }
+
+ private def visitDeclareConditionStatementImpl(
+ ctx: DeclareConditionStatementContext): ErrorCondition = {
+ val conditionName = ctx.multipartIdentifier().getText
Review Comment:
There are two question in one here:
1. As Milan point out we do allow sub-conditions for Spark conditions. Note
that the Standard does not have this (or global conditions for that matter at
all). So it's our choice to extend this to local conditions. I would say no
that appears to be over-engineered for a short lived local object. If we ever
wanted CREATE CONDITION, then we could revisit.
2. We will absolutely have to support raising Spark sub-conditions in
SIGNAL, RESIGNAL, and condition handlers
3. In the standard condition_name is a simple identifier and not qualified.
So it stands to reason that:
a) You cannot reference a condition by label.condition
b)You cannot locally overload a condition defined in an outer scope. The
standard says (15.3 3) At most one <condition declaration> shall specify a
<condition name>
that is equivalent to CN and has the same scope as CN.
But it uses the same language for handler declaration in 15.2 4):
No other <handler declaration> with the same scope as HD shall contain in
its <condition value list> a <condition value> that represents the same
condition as a <condition value> contained in the <condition value list> of HD.
Clearly though you can overload handlers, as there is a whole section on
"most appropriate handler"
So for now I propose this:
1. We parse multi part condition names, but we allow them only on SIGNAL,
RESIGNAL, and in handler declaration. And then only if they match to Spark
subconditions.
2. We block overloading of local conditions (unlike variables) for now. We
can't prevent overloading ov Spark conditions, that would cause future
breakages as we don not have a dedicated namespace.
--
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
