[SPARK-30759][SQL] Initialize cache for foldable patterns in StringRegexExpression #27502

MaxGekk · 2020-02-08T20:32:34Z

What changes were proposed in this pull request?

In the PR, I propose to fix cache initialization in StringRegexExpression by changing case Literal(value: String, StringType) to case p: Expression if p.foldable

Why are the changes needed?

Actually, the case doesn't work at all because of:

Literals value has type UTF8String
It doesn't work for foldable expressions like in the example:

SELECT '%SystemDrive%\Users\John' _FUNC_ '%SystemDrive%\\Users.*';

Does this PR introduce any user-facing change?

No

How was this patch tested?

By the check outputs of expression examples test from SQLQuerySuite.

dongjoon-hyun · 2020-02-08T23:31:05Z

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

-  // try cache the pattern for Literal
+  // try cache foldable pattern
  private lazy val cache: Pattern = pattern match {
-    case Literal(value: String, StringType) => compile(value)


@MaxGekk . This is SPARK-9242 (Spark 1.5.0) https://github.com/apache/spark/pull/8321/files#diff-39298b470865a4cbc67398a4ea11e767R41.

Reynold just moved the code. Actually it exists since 1.0.0: af3746c#diff-d788f93e29b4d25cdd7d60328587678bR42

dongjoon-hyun · 2020-02-08T23:32:35Z

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

  private lazy val cache: Pattern = pattern match {
-    case Literal(value: String, StringType) => compile(value)
-    case _ => null
+    case p: Expression if p.foldable =>


This will consider 'a' + 'b' from now. I prefer to consider SPARK-30759 as a performance improvement and merge to master only.

The changes don't impact on behavior in any case, so, they can be considered only as an optimization.

dongjoon-hyun · 2020-02-08T23:34:46Z

cc @rxin, @gatorsmile , @cloud-fan .

@MaxGekk . Although the above is my first impression, I'll not be against backporting this.

SparkQA · 2020-02-09T00:53:40Z

Test build #118075 has finished for PR 27502 at commit 0986e5c.

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

MaxGekk · 2020-02-10T07:45:00Z

@HyukjinKwon FYI, since you are in #26875 & #27514

HyukjinKwon

Looks good. Yes, my impression is that we don't need to back port.

SparkQA · 2020-02-10T08:05:01Z

Test build #118126 has finished for PR 27502 at commit 7bc5d9e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-02-10T08:06:46Z

retest this please

SparkQA · 2020-02-10T12:29:13Z

Test build #118139 has finished for PR 27502 at commit 7bc5d9e.

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

SparkQA · 2020-02-10T13:17:54Z

Test build #118135 has finished for PR 27502 at commit 7bc5d9e.

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

dongjoon-hyun

Thank you, @MaxGekk and @HyukjinKwon .
+1, LGTM. Merged to master for 3.1.0.

cloud-fan · 2020-02-11T04:04:44Z

Good catch! I'm surprised that this bug is exposed after so many years...

Shall we add a test for this bug?

dongjoon-hyun · 2020-02-11T18:19:25Z

+1 for @cloud-fan 's suggestion.

MaxGekk · 2020-02-12T09:35:23Z

@dongjoon-hyun @cloud-fan Here is the test #27547

…sion ### What changes were proposed in this pull request? In the PR, I propose to fix `cache` initialization in `StringRegexExpression` by changing of expected value type in `case Literal(value: String, StringType)` from `String` to `UTF8String`. This is a backport of #27502 and #27547 ### Why are the changes needed? Actually, the case doesn't work at all because `Literal`'s value has type `UTF8String`, see <img width="649" alt="Screen Shot 2020-02-08 at 22 45 50" src="https://user-images.githubusercontent.com/1580697/74091681-0d4a2180-4acb-11ea-8a0d-7e8c65f4214e.png"> ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added new test by `RegexpExpressionsSuite`. Closes #27713 from MaxGekk/str-regexp-foldable-pattern-backport. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…sion In the PR, I propose to fix `cache` initialization in `StringRegexExpression` by changing of expected value type in `case Literal(value: String, StringType)` from `String` to `UTF8String`. This is a backport of #27502 and #27547 Actually, the case doesn't work at all because `Literal`'s value has type `UTF8String`, see <img width="649" alt="Screen Shot 2020-02-08 at 22 45 50" src="https://user-images.githubusercontent.com/1580697/74091681-0d4a2180-4acb-11ea-8a0d-7e8c65f4214e.png"> No Added new test by `RegexpExpressionsSuite`. Closes #27713 from MaxGekk/str-regexp-foldable-pattern-backport. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit cfc48a8) Signed-off-by: Dongjoon Hyun <[email protected]>

…gexExpression ### What changes were proposed in this pull request? In the PR, I propose to fix `cache` initialization in `StringRegexExpression` by changing `case Literal(value: String, StringType)` to `case p: Expression if p.foldable` ### Why are the changes needed? Actually, the case doesn't work at all because of: 1. Literals value has type `UTF8String` 2. It doesn't work for foldable expressions like in the example: ```sql SELECT '%SystemDrive%\Users\John' _FUNC_ '%SystemDrive%\\Users.*'; ``` <img width="649" alt="Screen Shot 2020-02-08 at 22 45 50" src="https://user-images.githubusercontent.com/1580697/74091681-0d4a2180-4acb-11ea-8a0d-7e8c65f4214e.png"> ### Does this PR introduce any user-facing change? No ### How was this patch tested? By the `check outputs of expression examples` test from `SQLQuerySuite`. Closes apache#27502 from MaxGekk/str-regexp-foldable-pattern. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

MaxGekk added 2 commits February 8, 2020 22:49

Fix cache initialization for foldable expressions

b2cbe32

Fix comment

0986e5c

dongjoon-hyun added the SQL label Feb 8, 2020

dongjoon-hyun reviewed Feb 8, 2020

View reviewed changes

Avoid unnecessary changes

7bc5d9e

HyukjinKwon approved these changes Feb 10, 2020

View reviewed changes

dongjoon-hyun approved these changes Feb 10, 2020

View reviewed changes

dongjoon-hyun closed this in 3c1c9b4 Feb 10, 2020

MaxGekk mentioned this pull request Feb 26, 2020

[SPARK-30759][SQL][3.0] Fix cache initialization in StringRegexExpression #27713

Closed

MaxGekk deleted the str-regexp-foldable-pattern branch June 5, 2020 19:43

[SPARK-30759][SQL] Initialize cache for foldable patterns in StringRegexExpression #27502

[SPARK-30759][SQL] Initialize cache for foldable patterns in StringRegexExpression #27502

Uh oh!

Conversation

MaxGekk commented Feb 8, 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?

Uh oh!

dongjoon-hyun Feb 8, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 9, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Feb 8, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 9, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 9, 2020

Uh oh!

MaxGekk commented Feb 10, 2020

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 10, 2020

Uh oh!

HyukjinKwon commented Feb 10, 2020

Uh oh!

SparkQA commented Feb 10, 2020

Uh oh!

SparkQA commented Feb 10, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 11, 2020

Uh oh!

dongjoon-hyun commented Feb 11, 2020

Uh oh!

MaxGekk commented Feb 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongjoon-hyun commented Feb 8, 2020 •

edited

Loading