[SPARK-30625][SQL] Support `escape` as third parameter of the `like` function #27355

MaxGekk · 2020-01-24T13:54:28Z

What changes were proposed in this pull request?

In the PR, I propose to transform the Like expression to TernaryExpression, and add third parameter escape. So, the like function will have feature parity with LIKE ... ESCAPE syntax supported by 187f3c1.

Why are the changes needed?

The like functions can be called with 2 or 3 parameters, and functionally equivalent to LIKE and LIKE ... ESCAPE SQL expressions.

Does this PR introduce any user-facing change?

Yes, before like fails with the exception:

spark-sql> SELECT like('_Apache Spark_', '__%Spark__', '_');
Error in query: Invalid number of arguments for function like. Expected: 2; Found: 3; line 1 pos 7

After:

spark-sql> SELECT like('_Apache Spark_', '__%Spark__', '_');
true

How was this patch tested?

Add new example for the like function which is checked by SQLQuerySuite
Run RegexpExpressionsSuite and ExpressionParserSuite.

dongjoon-hyun

Ur, actually, I expected a smaller change.

MaxGekk · 2020-01-24T17:53:43Z

Actually I did bigger change but reverted my refactoring in RLike. The change could be smaller if I didn't rename expression parameters but I thought case class Like(left: Expression, right: Expression, escapeChar: Char) looks slightly confusing since it is not binary expression any more, and right is not most right parameter.

SparkQA · 2020-01-24T18:01:22Z

Test build #117356 has finished for PR 27355 at commit 203d1fa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Like(str: Expression, pattern: Expression, escape: Expression)
case class RLike(left: Expression, right: Expression)

maropu · 2020-01-26T12:01:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala

+  override def inputTypes: Seq[DataType] = Seq(StringType, StringType, StringType)
+  override def children: Seq[Expression] = Seq(str, pattern, escape)
+
+  private lazy val escapeChar: Char = if (escape.foldable) {


We need lazy here? I personally think its better to check error conditions as soon as possible.

When I remove lazy, I get the exception:

makeCopy, tree: str#13580 LIKE pattern#13581 ESCAPE '@' org.apache.spark.sql.catalyst.errors.package$TreeNodeException: makeCopy, tree: str#13580 LIKE pattern#13581 ESCAPE '@' at org.apache.spark.sql.catalyst.errors.package$.attachTree(package.scala:56) at org.apache.spark.sql.catalyst.trees.TreeNode.makeCopy(TreeNode.scala:435) at org.apache.spark.sql.catalyst.trees.TreeNode.mapChildren(TreeNode.scala:408) ... Caused by: org.apache.spark.sql.AnalysisException: The 'escape' parameter must be a string literal.; at org.apache.spark.sql.catalyst.expressions.Like.<init>(regexpExpressions.scala:135)

See escape is PrettyAttribute:

I got the error when I run new test in DataFrameFunctionsSuite, and I remember I got the error on other tests as well. PrettyAttribute is not foldable.

Ur, ok. Thanks for the check. The current one looks ok to me.

maropu · 2020-01-26T12:02:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala

+        s"The 'escape' parameter must be a string literal of one char but it is $s.")
+    }
+  } else {
+    throw new AnalysisException("The 'escape' parameter must be a string literal.")


Can you add tests for this path and line 131 (error cases)?

maropu · 2020-01-26T12:03:36Z

The refactoring still looks fine to me.

This reverts commit e58486a.

SparkQA · 2020-01-26T21:06:56Z

Test build #117428 has finished for PR 27355 at commit 39e4bd2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu

cc: @dongjoon-hyun @HyukjinKwon

dongjoon-hyun

+1, LGTM. Thank you, @MaxGekk and @maropu .
Merged to master.

rednaxelafx · 2020-02-10T20:43:05Z

Sorry for posting this review post-hoc, but I strongly believe this PR should be reverted for now, and re-land after redesign.

There are both design and implementation issues with this PR:

Design: in order to get to feature parity between the LIKE syntax and the like built-in function, the latter should only accept Char argument for the escapeChar parameter, instead of the full-blown Expression -- the LIKE syntax only accepts a literal anyway. c.f.

spark/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

Line 748 in a7451f4

| NOT? kind=LIKE pattern=valueExpression (ESCAPE escapeChar=STRING)?

By not changing the escapeChar parameter to an Expression, we can avoid most of the complexity (and bugs) in this PR.
Implementation. The shim of escapeChar = ... to adapt the Expression argument back to a Char is very bad. See this example:

Seq(("abc", "%bc", "/")).toDF("s", "p", "esc").selectExpr("like(s, p, esc)").explain(true)

and on a freshly built Apache Spark master, you'll get:

== Parsed Logical Plan ==
'Project [unresolvedalias('like('s, 'p, 'esc), Some(org.apache.spark.sql.Column$$Lambda$2247/782587714@5cf3924c))]
+- Project [_1#3 AS s#10, _2#4 AS p#11, _3#5 AS esc#12]
   +- LocalRelation [_1#3, _2#4, _3#5]

== Analyzed Logical Plan ==
s LIKE p: boolean
org.apache.spark.sql.AnalysisException: The 'escape' parameter must be a string literal.;
== Optimized Logical Plan ==
org.apache.spark.sql.AnalysisException: The 'escape' parameter must be a string literal.;
== Physical Plan ==
org.apache.spark.sql.AnalysisException: The 'escape' parameter must be a string literal.;

It doesn't look like supporting full-blown Expression for escapeChar is worth the cost here.

EDIT: to make my comment clear: I do think it's reasonable to allow the builtin like() function have feature parity with what we support in the syntax, i.e. accepting a literal argument for escapeChar. So the underlying intent of this PR sounds good to me. It's just the details that don't look quite right.

In order to support a 3-arg version of like() builtin function, we don't really have to change the signature of the primary constructor of Like. Instead, adding a constructor overload that takes the 3rd argument as an Expression and asserting that it only supports Literal or foldable would do.
That way we can have feature parity without most of the changes in this PR.

gatorsmile · 2020-02-10T20:57:40Z

Which systems support this syntax?

dongjoon-hyun · 2020-02-10T23:33:17Z

Thank you for pointing that, @rednaxelafx .

dongjoon-hyun · 2020-02-10T23:34:44Z

According to the above comment, @MaxGekk is trying to revert this first from 3.0.0 and will target 3.1.0 later.

cloud-fan · 2020-02-11T03:46:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala

+        s"The 'escape' parameter must be a string literal of one char but it is $s.")
+    }
+  } else {
+    throw new AnalysisException("The 'escape' parameter must be a string literal.")


This kind of thing should be done in checkInputDataTypes.

… `like` function ### What changes were proposed in this pull request? In the PR, I propose to revert the commit 8aebc80. ### Why are the changes needed? See the concerns #27355 (comment) ### Does this PR introduce any user-facing change? No ### How was this patch tested? By existing test suites. Closes #27531 from MaxGekk/revert-like-3-args. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

… `like` function In the PR, I propose to revert the commit 8aebc80. See the concerns #27355 (comment) No By existing test suites. Closes #27531 from MaxGekk/revert-like-3-args. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

… `like` function ### What changes were proposed in this pull request? In the PR, I propose to revert the commit 8aebc80. ### Why are the changes needed? See the concerns apache#27355 (comment) ### Does this PR introduce any user-facing change? No ### How was this patch tested? By existing test suites. Closes apache#27531 from MaxGekk/revert-like-3-args. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Support escape as third parameter of the like function

203d1fa

MaxGekk requested review from dongjoon-hyun and maropu January 24, 2020 13:54

dongjoon-hyun added the SQL label Jan 24, 2020

dongjoon-hyun reviewed Jan 24, 2020

View reviewed changes

maropu reviewed Jan 26, 2020

View reviewed changes

MaxGekk added 3 commits January 26, 2020 19:06

Add a test for long escape

e58486a

Negative tests for the like function

a4caad3

Revert "Add a test for long escape"

39e4bd2

This reverts commit e58486a.

maropu approved these changes Jan 27, 2020

View reviewed changes

dongjoon-hyun approved these changes Jan 27, 2020

View reviewed changes

dongjoon-hyun closed this in 8aebc80 Jan 27, 2020

MaxGekk mentioned this pull request Feb 10, 2020

Revert "[SPARK-30625][SQL] Support escape as third parameter of the like function #27531

Closed

cloud-fan reviewed Feb 11, 2020

View reviewed changes

MaxGekk deleted the like-3-args branch June 5, 2020 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30625][SQL] Support `escape` as third parameter of the `like` function #27355

[SPARK-30625][SQL] Support `escape` as third parameter of the `like` function #27355

MaxGekk commented Jan 24, 2020

dongjoon-hyun left a comment

MaxGekk commented Jan 24, 2020

SparkQA commented Jan 24, 2020

maropu Jan 26, 2020

MaxGekk Jan 26, 2020

MaxGekk Jan 26, 2020 •

edited

Loading

maropu Jan 27, 2020

maropu Jan 26, 2020

MaxGekk Jan 26, 2020

maropu commented Jan 26, 2020

SparkQA commented Jan 26, 2020

maropu left a comment

dongjoon-hyun left a comment

rednaxelafx commented Feb 10, 2020 •

edited

Loading

gatorsmile commented Feb 10, 2020

dongjoon-hyun commented Feb 10, 2020

dongjoon-hyun commented Feb 10, 2020

cloud-fan Feb 11, 2020

[SPARK-30625][SQL] Support escape as third parameter of the like function #27355

[SPARK-30625][SQL] Support escape as third parameter of the like function #27355

Conversation

MaxGekk commented Jan 24, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

dongjoon-hyun left a comment

Choose a reason for hiding this comment

MaxGekk commented Jan 24, 2020

SparkQA commented Jan 24, 2020

maropu Jan 26, 2020

Choose a reason for hiding this comment

MaxGekk Jan 26, 2020

Choose a reason for hiding this comment

MaxGekk Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

maropu Jan 27, 2020

Choose a reason for hiding this comment

maropu Jan 26, 2020

Choose a reason for hiding this comment

MaxGekk Jan 26, 2020

Choose a reason for hiding this comment

maropu commented Jan 26, 2020

SparkQA commented Jan 26, 2020

maropu left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

rednaxelafx commented Feb 10, 2020 • edited Loading

gatorsmile commented Feb 10, 2020

dongjoon-hyun commented Feb 10, 2020

dongjoon-hyun commented Feb 10, 2020

cloud-fan Feb 11, 2020

Choose a reason for hiding this comment

[SPARK-30625][SQL] Support `escape` as third parameter of the `like` function #27355

[SPARK-30625][SQL] Support `escape` as third parameter of the `like` function #27355

MaxGekk Jan 26, 2020 •

edited

Loading

rednaxelafx commented Feb 10, 2020 •

edited

Loading