Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
MaxGekk wants to merge 3 commits into from
Closed

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

MaxGekk wants to merge 3 commits into from

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Feb 10, 2020

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.

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 10, 2020

@rednaxelafx @gatorsmile Please, review this PR. I haven't run tests locally, let's wait for jenkins.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 10, 2020

Thank you for making a swift action, @MaxGekk .

rednaxelafx
rednaxelafx reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@rednaxelafx rednaxelafx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response, @MaxGekk !
This revert mostly LGTM except for one inline comment below:

@SparkQA
Copy link

SparkQA commented Feb 11, 2020

Test build #118181 has finished for PR 27531 at commit fc366a8.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class Like(left: Expression, right: Expression, escapeChar: Char)
  • case class RLike(left: Expression, right: Expression) extends StringRegexExpression

HyukjinKwon
HyukjinKwon approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine given the discussion made in the linked PR.

@SparkQA
Copy link

SparkQA commented Feb 11, 2020

Test build #118211 has finished for PR 27531 at commit 1e7035c.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 11, 2020

jenkins, retest this, please

rednaxelafx
rednaxelafx approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@rednaxelafx rednaxelafx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after the update. Thanks @MaxGekk !

@MaxGekk
Merge remote-tracking branch 'remotes/origin/master' into revert-like…
ff569c4 
…-3-args

# Conflicts:
#	sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala
@SparkQA
Copy link

SparkQA commented Feb 11, 2020

Test build #118218 has finished for PR 27531 at commit 1e7035c.

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

@SparkQA
Copy link

SparkQA commented Feb 11, 2020

Test build #118231 has finished for PR 27531 at commit ff569c4.

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

dongjoon-hyun
dongjoon-hyun reviewed Feb 11, 2020
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala
String $rightStr = $eval2.toString();
$patternClass $pattern = $patternClass.compile(
$escapeFunc($rightStr, '$newEscapeChar'));
${ev.value} = $pattern.matcher($eval1.toString()).matches();
Copy link
Member

@dongjoon-hyun dongjoon-hyun Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for me: This part is a logical reverting.

dongjoon-hyun
dongjoon-hyun approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you all
Merged to master.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 11, 2020 •
edited
Loading

Could you make a backport to branch-3.0, please, @MaxGekk ? There is a conflict.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 12, 2020

Actually that conflict is by me #27514. Let me handle it.

HyukjinKwon pushed a commit that referenced this pull request Feb 12, 2020
@MaxGekk @HyukjinKwon
Revert "[SPARK-30625][SQL] Support escape as third parameter of the…
9c73935 
… `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 <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 12, 2020

Thank you, @HyukjinKwon !

sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
@MaxGekk
Revert "[SPARK-30625][SQL] Support escape as third parameter of the…
68accbb 
… `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 <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@MaxGekk MaxGekk deleted the revert-like-3-args branch June 5, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

@gatorsmile gatorsmile Awaiting requested review from gatorsmile

+1 more reviewer

@rednaxelafx rednaxelafx rednaxelafx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@MaxGekk @dongjoon-hyun @SparkQA @HyukjinKwon @rednaxelafx @maropu