This is an automated email from the ASF dual-hosted git repository. dongjoon pushed a commit to branch branch-2.3 in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-2.3 by this push: new 1201a0a [SPARK-28093][SPARK-28109][SQL][2.3] Fix TRIM/LTRIM/RTRIM function parameter order issue 1201a0a is described below commit 1201a0a7580a8c41d0501c95826c900f84e1db45 Author: Yuming Wang <yumw...@ebay.com> AuthorDate: Fri Jun 21 18:40:23 2019 -0700 [SPARK-28093][SPARK-28109][SQL][2.3] Fix TRIM/LTRIM/RTRIM function parameter order issue ## What changes were proposed in this pull request? This pr backport #24902 and #24911 to branch-2.3. ## How was this patch tested? unit tests Closes #24908 from wangyum/SPARK-28093-branch-2.3. Authored-by: Yuming Wang <yumw...@ebay.com> Signed-off-by: Dongjoon Hyun <dh...@apple.com> --- .../apache/spark/sql/catalyst/parser/SqlBase.g4 | 6 +- .../catalyst/expressions/stringExpressions.scala | 6 +- .../spark/sql/catalyst/parser/AstBuilder.scala | 43 +++++++------- .../expressions/StringExpressionsSuite.scala | 11 ++++ .../sql/catalyst/parser/PlanParserSuite.scala | 24 +++++--- .../parser/TableIdentifierParserSuite.scala | 2 +- .../sql-tests/inputs/string-functions.sql | 12 +++- .../sql-tests/results/string-functions.sql.out | 66 +++++++++++++++++++++- 8 files changed, 130 insertions(+), 40 deletions(-) diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index 5fa75fe..15fd48b 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -582,12 +582,12 @@ primaryExpression | '(' query ')' #subqueryExpression | qualifiedName '(' (setQuantifier? argument+=expression (',' argument+=expression)*)? ')' (OVER windowSpec)? #functionCall - | qualifiedName '(' trimOption=(BOTH | LEADING | TRAILING) argument+=expression - FROM argument+=expression ')' #functionCall | value=primaryExpression '[' index=valueExpression ']' #subscript | identifier #columnReference | base=primaryExpression '.' fieldName=identifier #dereference | '(' expression ')' #parenthesizedExpression + | TRIM '(' trimOption=(BOTH | LEADING | TRAILING) (trimStr=valueExpression)? + FROM srcStr=valueExpression ')' #trim ; constant @@ -735,6 +735,7 @@ nonReserved | VIEW | REPLACE | IF | POSITION + | TRIM | NO | DATA | START | TRANSACTION | COMMIT | ROLLBACK | IGNORE | SORT | CLUSTER | DISTRIBUTE | UNSET | TBLPROPERTIES | SKEWED | STORED | DIRECTORIES | LOCATION @@ -872,6 +873,7 @@ TRAILING: 'TRAILING'; IF: 'IF'; POSITION: 'POSITION'; +TRIM: 'TRIM'; EQ : '=' | '=='; NSEQ: '<=>'; diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala index c855581..1166e77 100755 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala @@ -714,7 +714,7 @@ case class StringTrim( trimStr: Option[Expression] = None) extends String2TrimExpression { - def this(trimStr: Expression, srcStr: Expression) = this(srcStr, Option(trimStr)) + def this(srcStr: Expression, trimStr: Expression) = this(srcStr, Option(trimStr)) def this(srcStr: Expression) = this(srcStr, None) @@ -814,7 +814,7 @@ case class StringTrimLeft( trimStr: Option[Expression] = None) extends String2TrimExpression { - def this(trimStr: Expression, srcStr: Expression) = this(srcStr, Option(trimStr)) + def this(srcStr: Expression, trimStr: Expression) = this(srcStr, Option(trimStr)) def this(srcStr: Expression) = this(srcStr, None) @@ -917,7 +917,7 @@ case class StringTrimRight( trimStr: Option[Expression] = None) extends String2TrimExpression { - def this(trimStr: Expression, srcStr: Expression) = this(srcStr, Option(trimStr)) + def this(srcStr: Expression, trimStr: Expression) = this(srcStr, Option(trimStr)) def this(srcStr: Expression) = this(srcStr, None) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index bdc357d..418e029 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -1186,29 +1186,28 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging } /** + * Create a Trim expression. + */ + override def visitTrim(ctx: TrimContext): Expression = withOrigin(ctx) { + val srcStr = expression(ctx.srcStr) + val trimStr = Option(ctx.trimStr).map(expression) + ctx.trimOption.getType match { + case SqlBaseParser.BOTH => + StringTrim(srcStr, trimStr) + case SqlBaseParser.LEADING => + StringTrimLeft(srcStr, trimStr) + case SqlBaseParser.TRAILING => + StringTrimRight(srcStr, trimStr) + case other => + throw new ParseException("Function trim doesn't support with " + + s"type $other. Please use BOTH, LEADING or TRAILING as trim type", ctx) + } + } + + /** * Create a (windowed) Function expression. */ override def visitFunctionCall(ctx: FunctionCallContext): Expression = withOrigin(ctx) { - def replaceFunctions( - funcID: FunctionIdentifier, - ctx: FunctionCallContext): FunctionIdentifier = { - val opt = ctx.trimOption - if (opt != null) { - if (ctx.qualifiedName.getText.toLowerCase(Locale.ROOT) != "trim") { - throw new ParseException(s"The specified function ${ctx.qualifiedName.getText} " + - s"doesn't support with option ${opt.getText}.", ctx) - } - opt.getType match { - case SqlBaseParser.BOTH => funcID - case SqlBaseParser.LEADING => funcID.copy(funcName = "ltrim") - case SqlBaseParser.TRAILING => funcID.copy(funcName = "rtrim") - case _ => throw new ParseException("Function trim doesn't support with " + - s"type ${opt.getType}. Please use BOTH, LEADING or Trailing as trim type", ctx) - } - } else { - funcID - } - } // Create the function call. val name = ctx.qualifiedName.getText val isDistinct = Option(ctx.setQuantifier()).exists(_.DISTINCT != null) @@ -1220,9 +1219,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging case expressions => expressions } - val funcId = replaceFunctions(visitFunctionName(ctx.qualifiedName), ctx) - val function = UnresolvedFunction(funcId, arguments, isDistinct) - + val function = UnresolvedFunction(visitFunctionName(ctx.qualifiedName), arguments, isDistinct) // Check if the function is evaluated in a windowed context. ctx.windowSpec match { diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala index 97ddbeb..45b93ec 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala @@ -466,6 +466,9 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { // scalastyle:on checkEvaluation(StringTrim(Literal("a"), Literal.create(null, StringType)), null) checkEvaluation(StringTrim(Literal.create(null, StringType), Literal("a")), null) + + checkEvaluation(StringTrim(Literal("yxTomxx"), Literal("xyz")), "Tom") + checkEvaluation(StringTrim(Literal("xxxbarxxx"), Literal("x")), "bar") } test("LTRIM") { @@ -490,6 +493,10 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { // scalastyle:on checkEvaluation(StringTrimLeft(Literal.create(null, StringType), Literal("a")), null) checkEvaluation(StringTrimLeft(Literal("a"), Literal.create(null, StringType)), null) + + checkEvaluation(StringTrimLeft(Literal("zzzytest"), Literal("xyz")), "test") + checkEvaluation(StringTrimLeft(Literal("zzzytestxyz"), Literal("xyz")), "testxyz") + checkEvaluation(StringTrimLeft(Literal("xyxXxyLAST WORD"), Literal("xy")), "XxyLAST WORD") } test("RTRIM") { @@ -515,6 +522,10 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { // scalastyle:on checkEvaluation(StringTrimRight(Literal("a"), Literal.create(null, StringType)), null) checkEvaluation(StringTrimRight(Literal.create(null, StringType), Literal("a")), null) + + checkEvaluation(StringTrimRight(Literal("testxxzx"), Literal("xyz")), "test") + checkEvaluation(StringTrimRight(Literal("xyztestxxzx"), Literal("xyz")), "xyztest") + checkEvaluation(StringTrimRight(Literal("TURNERyxXxy"), Literal("xy")), "TURNERyxX") } test("FORMAT") { diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala index 812bfdd..687f5fa 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala @@ -18,7 +18,7 @@ package org.apache.spark.sql.catalyst.parser import org.apache.spark.sql.catalyst.{FunctionIdentifier, TableIdentifier} -import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, UnresolvedAttribute, UnresolvedFunction, UnresolvedGenerator, UnresolvedInlineTable, UnresolvedRelation, UnresolvedSubqueryColumnAliases, UnresolvedTableValuedFunction} +import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, UnresolvedAlias, UnresolvedAttribute, UnresolvedFunction, UnresolvedGenerator, UnresolvedInlineTable, UnresolvedRelation, UnresolvedSubqueryColumnAliases, UnresolvedTableValuedFunction} import org.apache.spark.sql.catalyst.expressions._ import org.apache.spark.sql.catalyst.plans._ import org.apache.spark.sql.catalyst.plans.logical._ @@ -652,20 +652,26 @@ class PlanParserSuite extends AnalysisTest { } test("TRIM function") { - intercept("select ltrim(both 'S' from 'SS abc S'", "missing ')' at '<EOF>'") - intercept("select rtrim(trailing 'S' from 'SS abc S'", "missing ')' at '<EOF>'") + def assertTrimPlans(inputSQL: String, expectedExpression: Expression): Unit = { + comparePlans( + parsePlan(inputSQL), + Project(Seq(UnresolvedAlias(expectedExpression)), OneRowRelation()) + ) + } + intercept("select ltrim(both 'S' from 'SS abc S'", "mismatched input 'from' expecting {')'") + intercept("select rtrim(trailing 'S' from 'SS abc S'", "mismatched input 'from' expecting {')'") - assertEqual( + assertTrimPlans( "SELECT TRIM(BOTH '@$%&( )abc' FROM '@ $ % & ()abc ' )", - OneRowRelation().select('TRIM.function("@$%&( )abc", "@ $ % & ()abc ")) + StringTrim(Literal("@ $ % & ()abc "), Some(Literal("@$%&( )abc"))) ) - assertEqual( + assertTrimPlans( "SELECT TRIM(LEADING 'c []' FROM '[ ccccbcc ')", - OneRowRelation().select('ltrim.function("c []", "[ ccccbcc ")) + StringTrimLeft(Literal("[ ccccbcc "), Some(Literal("c []"))) ) - assertEqual( + assertTrimPlans( "SELECT TRIM(TRAILING 'c&^,.' FROM 'bc...,,,&&&ccc')", - OneRowRelation().select('rtrim.function("c&^,.", "bc...,,,&&&ccc")) + StringTrimRight(Literal("bc...,,,&&&ccc"), Some(Literal("c&^,."))) ) } } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala index cc80a41..50e17b1 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala @@ -51,7 +51,7 @@ class TableIdentifierParserSuite extends SparkFunSuite { "rollup", "row", "rows", "set", "smallint", "table", "timestamp", "to", "trigger", "true", "truncate", "update", "user", "values", "with", "regexp", "rlike", "bigint", "binary", "boolean", "current_date", "current_timestamp", "date", "double", "float", - "int", "smallint", "timestamp", "at", "position", "both", "leading", "trailing") + "int", "smallint", "timestamp", "at", "position", "both", "leading", "trailing", "trim") val hiveStrictNonReservedKeyword = Seq("anti", "full", "inner", "left", "semi", "right", "natural", "union", "intersect", "except", "database", "on", "join", "cross", "select", "from", diff --git a/sql/core/src/test/resources/sql-tests/inputs/string-functions.sql b/sql/core/src/test/resources/sql-tests/inputs/string-functions.sql index 4113734..f63dbec 100644 --- a/sql/core/src/test/resources/sql-tests/inputs/string-functions.sql +++ b/sql/core/src/test/resources/sql-tests/inputs/string-functions.sql @@ -46,4 +46,14 @@ FROM ( encode(string(id + 2), 'utf-8') col3, encode(string(id + 3), 'utf-8') col4 FROM range(10) -) +); + +-- trim/ltrim/rtrim +SELECT trim('yxTomxx', 'xyz'), trim(BOTH 'xyz' FROM 'yxTomxx'); +SELECT trim('xxxbarxxx', 'x'), trim(BOTH 'x' FROM 'xxxbarxxx'); +SELECT ltrim('zzzytest', 'xyz'), trim(LEADING 'xyz' FROM 'zzzytest'); +SELECT ltrim('zzzytestxyz', 'xyz'), trim(LEADING 'xyz' FROM 'zzzytestxyz'); +SELECT ltrim('xyxXxyLAST WORD', 'xy'), trim(LEADING 'xy' FROM 'xyxXxyLAST WORD'); +SELECT rtrim('testxxzx', 'xyz'), trim(TRAILING 'xyz' FROM 'testxxzx'); +SELECT rtrim('xyztestxxzx', 'xyz'), trim(TRAILING 'xyz' FROM 'xyztestxxzx'); +SELECT rtrim('TURNERyxXxy', 'xy'), trim(TRAILING 'xy' FROM 'TURNERyxXxy'); diff --git a/sql/core/src/test/resources/sql-tests/results/string-functions.sql.out b/sql/core/src/test/resources/sql-tests/results/string-functions.sql.out index d5f8705..aace15f 100644 --- a/sql/core/src/test/resources/sql-tests/results/string-functions.sql.out +++ b/sql/core/src/test/resources/sql-tests/results/string-functions.sql.out @@ -1,5 +1,5 @@ -- Automatically generated by SQLQueryTestSuite --- Number of queries: 15 +-- Number of queries: 23 -- !query 0 @@ -161,3 +161,67 @@ struct<plan:string> == Physical Plan == *Project [concat(cast(id#xL as string), cast(encode(cast((id#xL + 2) as string), utf-8) as string), cast(encode(cast((id#xL + 3) as string), utf-8) as string)) AS col#x] +- *Range (0, 10, step=1, splits=2) + + +-- !query 15 +SELECT trim('yxTomxx', 'xyz'), trim(BOTH 'xyz' FROM 'yxTomxx') +-- !query 15 schema +struct<trim(yxTomxx, xyz):string,trim(yxTomxx, xyz):string> +-- !query 15 output +Tom Tom + + +-- !query 16 +SELECT trim('xxxbarxxx', 'x'), trim(BOTH 'x' FROM 'xxxbarxxx') +-- !query 16 schema +struct<trim(xxxbarxxx, x):string,trim(xxxbarxxx, x):string> +-- !query 16 output +bar bar + + +-- !query 17 +SELECT ltrim('zzzytest', 'xyz'), trim(LEADING 'xyz' FROM 'zzzytest') +-- !query 17 schema +struct<ltrim(zzzytest, xyz):string,ltrim(zzzytest, xyz):string> +-- !query 17 output +test test + + +-- !query 18 +SELECT ltrim('zzzytestxyz', 'xyz'), trim(LEADING 'xyz' FROM 'zzzytestxyz') +-- !query 18 schema +struct<ltrim(zzzytestxyz, xyz):string,ltrim(zzzytestxyz, xyz):string> +-- !query 18 output +testxyz testxyz + + +-- !query 19 +SELECT ltrim('xyxXxyLAST WORD', 'xy'), trim(LEADING 'xy' FROM 'xyxXxyLAST WORD') +-- !query 19 schema +struct<ltrim(xyxXxyLAST WORD, xy):string,ltrim(xyxXxyLAST WORD, xy):string> +-- !query 19 output +XxyLAST WORD XxyLAST WORD + + +-- !query 20 +SELECT rtrim('testxxzx', 'xyz'), trim(TRAILING 'xyz' FROM 'testxxzx') +-- !query 20 schema +struct<rtrim(testxxzx, xyz):string,rtrim(testxxzx, xyz):string> +-- !query 20 output +test test + + +-- !query 21 +SELECT rtrim('xyztestxxzx', 'xyz'), trim(TRAILING 'xyz' FROM 'xyztestxxzx') +-- !query 21 schema +struct<rtrim(xyztestxxzx, xyz):string,rtrim(xyztestxxzx, xyz):string> +-- !query 21 output +xyztest xyztest + + +-- !query 22 +SELECT rtrim('TURNERyxXxy', 'xy'), trim(TRAILING 'xy' FROM 'TURNERyxXxy') +-- !query 22 schema +struct<rtrim(TURNERyxXxy, xy):string,rtrim(TURNERyxXxy, xy):string> +-- !query 22 output +TURNERyxX TURNERyxX --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org