Skip to content

[SPARK-19555][SQL] Improve the performance of StringUtils.escapeLikeRegex method - #16893

Closed
lins05 wants to merge 2 commits into
apache:masterfrom
lins05:spark-19555-improve-escape-like-regex
Closed

[SPARK-19555][SQL] Improve the performance of StringUtils.escapeLikeRegex method#16893
lins05 wants to merge 2 commits into
apache:masterfrom
lins05:spark-19555-improve-escape-like-regex

Conversation

@lins05

@lins05lins05 commented Feb 11, 2017

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Copied from SPARK-19555 JIRA

Spark's StringUtils.escapeLikeRegex() method is written inefficiently, performing tons of object allocations due to the use zip(), flatMap() , and mkString. Instead, I think method should be rewritten in an imperative style using a Java string builder.

This method can become a performance bottleneck in cases where regex expressions are used with non-constant-foldable expressions (e.g. the regex expression comes from the data rather than being part of the query).

How was this patch tested?

Existing tests.

Performance Comparison

number of rowsbeforeafter
10M12s4.5s
100M120s45s

Perf testing code:

./bin/spark-shell --master "local-cluster[2,1,10240]"
defperf(f: =>Unit, n: Int=3):Unit= {
for (i <-0 to n -1) {
valbefore=System.currentTimeMillis
f
valafter=System.currentTimeMillis
println(s"function took ${after - before} ms")
}
}
valn=10000000valdf= sc.parallelize(0 to n, 2).map(i => (i.toString, "%9999")).toDF("a", "b").cache
df.count()
df.createOrReplaceTempView("df")
perf {
sql("select count(*) from df where a like b").show()
}

@SparkQA

Copy link
Copy Markdown

Test build #72733 has started for PR 16893 at commit e68eab0.

@JoshRosen

Copy link
Copy Markdown
Contributor

jenkins retest this please

@tejasapatiltejasapatil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this PR is about improving performance, would recommend trying out with while loop and a counter instead of for(). I ran your test snippet on my box:

with for():

function took 5287 ms
function took 5234 ms
function took 5149 ms

with while loop and counter:

function took 4762 ms
function took 4216 ms
function took 4212 ms

Here is my version with while loop and counter:

 def escapeLikeRegex(input: String): String = {
val builder = new StringBuilder("(?s)")
val length = input.length
var i = 0
var previousChar = ' '
while (i < length) {
val currentChar = input.charAt(i)
if (currentChar != '\\') {
val out = if (previousChar == '\\') {
currentChar match {
case '_' => "_"
case '%' => "%"
case _ => Pattern.quote("\\" + currentChar)
}
} else {
currentChar match {
case '_' => "."
case '%' => ".*"
case _ => Pattern.quote(Character.toString(currentChar))
}
}
builder.append(out)
}
previousChar = currentChar
i += 1
}
builder.toString()
}

@@ -27,21 +28,28 @@ object StringUtils {
// replace the % with .*, match 0 or more times with any character
def escapeLikeRegex(v: String): String = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: while you are at it, can you also make the var names better ?

v -> input
c -> currentChar
prev -> previousChar

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could further simplify this by replacing previousChar with a boolean, nextCharacterIsEscaped (or inEscape).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, I'm wrong: we can't quite do that simplification because the old code has a subtle bug related to backslash-escaping. Due to the complexity and terseness the old implementation, it's a little non-obvious to spot that the case (prev, '\\') => "" has the effect of always ignoring backslash characters, so this method is incapable of producing a backslash in its output. This is a problem if the user wants to write a LIKE pattern to match backslashes then this is impossible with the current code.

It turns out that this is covered by #15398, which also implements performance improvements for this code, so I guess this PR and JIRA is redundant :(

I thought #15398 had been merged / fixed by now, but I guess not.

@SparkQA

Copy link
Copy Markdown

Test build #72788 has finished for PR 16893 at commit e68eab0.

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

@HyukjinKwon

Copy link
Copy Markdown
Member

Hi all, where are we on this? Is this still active?

@HyukjinKwonHyukjinKwon mentioned this pull request May 17, 2017
@JoshRosen

Copy link
Copy Markdown
Contributor

@HyukjinKwon, we can close this because its optimizations were incorporated into SPARK-17647 (I ran the benchmarks to verify this, too). I'm going to resolve this JIRA as fixed by that one as well.

zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
## What changes were proposed in this pull request?
This PR proposes to close PRs ...
- inactive to the review comments more than a month
- WIP and inactive more than a month
- with Jenkins build failure but inactive more than a month
- suggested to be closed and no comment against that
- obviously looking inappropriate (e.g., Branch 0.5)
To make sure, I left a comment for each PR about a week ago and I could not have a response back from the author in these PRs below:
Closesapache#11129Closesapache#12085Closesapache#12162Closesapache#12419Closesapache#12420Closesapache#12491Closesapache#13762Closesapache#13837Closesapache#13851Closesapache#13881Closesapache#13891Closesapache#13959Closesapache#14091Closesapache#14481Closesapache#14547Closesapache#14557Closesapache#14686Closesapache#15594Closesapache#15652Closesapache#15850Closesapache#15914Closesapache#15918Closesapache#16285Closesapache#16389Closesapache#16652Closesapache#16743Closesapache#16893Closesapache#16975Closesapache#17001Closesapache#17088Closesapache#17119Closesapache#17272Closesapache#17971
Added:
Closesapache#17778Closesapache#17303Closesapache#17872
## How was this patch tested?
N/A
Author: hyukjinkwon <gurwls223@gmail.com>
Closesapache#18017 from HyukjinKwon/close-inactive-prs.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@lins05@SparkQA@JoshRosen@HyukjinKwon@tejasapatil