[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar #30625

luluorta · 2020-12-06T12:56:14Z

What changes were proposed in this pull request?

LikeSimplification rule does not work correctly for many cases that have patterns containing escape characters, for example:

SELECT s LIKE 'm%aca' ESCAPE '%' FROM t
SELECT s LIKE 'maacaa' ESCAPE 'a' FROM t

For simpilicy, this PR makes this rule just be skipped if pattern contains any escapeChar.

Why are the changes needed?

Result corrupt.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added Unit test.

luluorta · 2020-12-06T13:01:18Z

cc @cloud-fan @leanken

SparkQA · 2020-12-06T14:18:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36891/

SparkQA · 2020-12-06T14:52:14Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36891/

SparkQA · 2020-12-06T17:59:07Z

Test build #132290 has finished for PR 30625 at commit 95713ed.

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

maropu · 2020-12-07T01:26:41Z

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

      if (pattern == null) {
        // If pattern is null, return null value directly, since "col like null" == null.
        Literal(null, BooleanType)
      } else {
        val escapeStr = String.valueOf(escapeChar)
        pattern.toString match {
+          case _ if escapeChar == '_' || escapeChar == '%' => l


Please add tests in LikeSimplificationSuite, too? Btw, does this simplification work correctly if escapeChar is not a default one other than _ and %?

You are right! I think this rule works correctly only if pattern is valid and escapeChar does not escape itself. For simplicity, maybe it's better to skip it if pattern constains any escapeChar. I will fix this and change the title/description.

oh... I see...

cloud-fan · 2020-12-07T05:19:55Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+
+  test("SPARK-33677: LikeSimplification should be skipped if escape is a wildcard character") {
+    val e = intercept[AnalysisException] {
+      sql("SELECT string(rand()) LIKE 'm%aca' ESCAPE '%'").collect()


so the problem is, we should only apply like simplification if the pattern is valid?

This rule may not work even for the case that has a valid pattern: SELECT s LIKE 'maacaa' ESCAPE 'a' FROM t

…ny escapeChar

SparkQA · 2020-12-07T07:37:55Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36933/

cloud-fan · 2020-12-07T08:04:42Z

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

        pattern.toString match {
-          case startsWith(prefix) if !prefix.endsWith(escapeStr) =>
+          case p if p.contains(escapeChar) => l


correct me if I was wrong: we need to make sure this rule doesn't change result. If the pattern is invalid, Like should fail. However, it's expensive to validate the pattern (need to compile it). Here we use p.contains(escapeChar) as a shortcut to know if the pattern might be invalid.

Can we add some comments to explain it?

SparkQA · 2020-12-07T08:14:53Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36933/

SparkQA · 2020-12-07T11:35:32Z

Test build #132333 has finished for PR 30625 at commit 65e0d63.

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

SparkQA · 2020-12-07T13:22:03Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36966/

maropu · 2020-12-07T13:34:09Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+
+  test("SPARK-33677: LikeSimplification should be skipped if pattern contains any escapeChar") {
+    withTable("string_tbl") {
+      sql("CREATE TABLE string_tbl USING parquet SELECT 'm@ca' AS s")


nit: could you use a temporary ~~table~~ view for better test performance?

there is no temporary table in spark, you mean temporary view?

Unfortunately Like will be constant folded and not passed to LikeSimplification if using temp view here.

Oh, yea, I meant a temporary view...

@luluorta have you tried temp view? AFAIK ConvertToLocalRelation is disabled in tests.

It works if I use DataFrame to create temp view.

maropu

Nice catch!

SparkQA · 2020-12-07T13:46:38Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36966/

SparkQA · 2020-12-07T17:13:48Z

Test build #132366 has finished for PR 30625 at commit 765591c.

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

SparkQA · 2020-12-08T03:29:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37001/

SparkQA · 2020-12-08T03:57:00Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37001/

SparkQA · 2020-12-08T04:38:16Z

Test build #132401 has finished for PR 30625 at commit 58b8bd2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-12-08T06:16:05Z

retest this please

SparkQA · 2020-12-08T07:18:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37013/

SparkQA · 2020-12-08T07:46:45Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37013/

SparkQA · 2020-12-08T10:52:57Z

Test build #132413 has finished for PR 30625 at commit 58b8bd2.

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

…ny escapeChar ### What changes were proposed in this pull request? `LikeSimplification` rule does not work correctly for many cases that have patterns containing escape characters, for example: `SELECT s LIKE 'm%aca' ESCAPE '%' FROM t` `SELECT s LIKE 'maacaa' ESCAPE 'a' FROM t` For simpilicy, this PR makes this rule just be skipped if `pattern` contains any `escapeChar`. ### Why are the changes needed? Result corrupt. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added Unit test. Closes #30625 from luluorta/SPARK-33677. Authored-by: luluorta <luluorta@gmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org> (cherry picked from commit 99613cd) Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

maropu · 2020-12-08T11:46:40Z

Thanks! Merged to master/branch-3.1/branch-3.0.

github-actions bot added the SQL label Dec 6, 2020

maropu reviewed Dec 7, 2020

View reviewed changes

cloud-fan reviewed Dec 7, 2020

View reviewed changes

[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains a…

65e0d63

…ny escapeChar

luluorta force-pushed the SPARK-33677 branch from 95713ed to 65e0d63 Compare December 7, 2020 06:00

luluorta changed the title ~~[SPARK-33677][SQL] Skip LikeSimplification rule if escape is a wildcard character~~ [SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar Dec 7, 2020

cloud-fan reviewed Dec 7, 2020

View reviewed changes

add comment

765591c

cloud-fan approved these changes Dec 7, 2020

View reviewed changes

maropu reviewed Dec 7, 2020

View reviewed changes

maropu approved these changes Dec 7, 2020

View reviewed changes

use temp view in test

58b8bd2

maropu closed this in 99613cd Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar #30625

[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar #30625

luluorta commented Dec 6, 2020 •

edited

Loading

luluorta commented Dec 6, 2020

SparkQA commented Dec 6, 2020

SparkQA commented Dec 6, 2020

SparkQA commented Dec 6, 2020

maropu Dec 7, 2020

luluorta Dec 7, 2020 •

edited

Loading

maropu Dec 7, 2020

cloud-fan Dec 7, 2020

luluorta Dec 7, 2020

SparkQA commented Dec 7, 2020

cloud-fan Dec 7, 2020

luluorta Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

maropu Dec 7, 2020 •

edited

Loading

cloud-fan Dec 7, 2020

luluorta Dec 7, 2020 •

edited

Loading

maropu Dec 7, 2020

cloud-fan Dec 7, 2020

luluorta Dec 8, 2020

maropu left a comment

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

cloud-fan commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

maropu commented Dec 8, 2020

[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar #30625

[SPARK-33677][SQL] Skip LikeSimplification rule if pattern contains any escapeChar #30625

Conversation

luluorta commented Dec 6, 2020 • edited Loading

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?

luluorta commented Dec 6, 2020

SparkQA commented Dec 6, 2020

SparkQA commented Dec 6, 2020

SparkQA commented Dec 6, 2020

Choose a reason for hiding this comment

luluorta Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

maropu Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luluorta Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

cloud-fan commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

SparkQA commented Dec 8, 2020

maropu commented Dec 8, 2020

luluorta commented Dec 6, 2020 •

edited

Loading

luluorta Dec 7, 2020 •

edited

Loading

maropu Dec 7, 2020 •

edited

Loading

luluorta Dec 7, 2020 •

edited

Loading