You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2024/10/30 03:28:10 UTC
[PR] [SPARK-50169][SQL] Improve performance of RegExpReplace [spark]
panbingkun opened a new pull request, #48702:
URL: https://github.com/apache/spark/pull/48702
### What changes were proposed in this pull request?
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA.
Manually check.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823704186
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
Because in Spark, `each task` is an independent thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "zeal-thinker (via GitHub)" <gi...@apache.org>.
zeal-thinker commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823809372
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
Oh okay! There are some other places in the codebase where we use StringBuffer. We may be able to use StringBuilder there and see some performance gain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "zeal-thinker (via GitHub)" <gi...@apache.org>.
zeal-thinker commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823144224
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
Why do we not need to use thread safe StringBuffer? Will this code be never run in a multithreaded environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2448879434
JDK 17:
JDK 21:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of RegExpReplace [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2445756443
- RegExpReplaceBenchmark
```scala
object RegExpReplaceBenchmark extends SqlBasedBenchmark {
private val N = 1000000 // 10_000_00
private val M = 100
private val df = spark.range(N).to(new StructType().add("id", "int")).
withColumn("subject", concat(col("id"), lit("-"), col("id") * 2)).
withColumn("regexp", lit("(\\d+)")).
withColumn("rep", lit("num"))
private def doBenchmark(): Unit = {
df.selectExpr("regexp_replace(subject, regexp, rep)").noop()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
runBenchmark("regexp_replace") {
val benchmark = new Benchmark("regexp_replace", N, output = output)
benchmark.addCase("optimize", M) { _ =>
doBenchmark()
}
benchmark.run()
}
}
}
```
- Before
```shell
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 40159 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 389 402 12 2.6 389.4 1.0X
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 41644 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 406 416 14 2.5 405.5 1.0X
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 39469 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 385 395 11 2.6 385.0 1.0X
```
- After
```
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 35651 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 341 357 23 2.9 340.9 1.0X
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 37869 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 368 379 12 2.7 367.8 1.0X
Running benchmark: regexp_replace
Running case: optimize
Stopped after 100 iterations, 34783 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.1
Apple M2
regexp_replace: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 328 348 16 3.0 328.2 1.0X
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823893660
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
I noticed another function `FormatNumber`, but according to the results of my local benchmark test, unfortunately, I did not see great performance gain (`almost no change`).
https://github.com/apache/spark/blob/beea9c36725e7aa8f17f952da70e1d25013dc336/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L3345
```scala
object FormatNumberBenchmark extends SqlBasedBenchmark {
private val N = 1000000 // 10_000_00
private val M = 100
private val df = spark.range(N).to(new StructType().add("id", "int")).
withColumn("format1", lit("##################.##")).
withColumn("format2", lit(4)).
withColumn("value", col("id") / M)
private def doBenchmark(): Unit = {
// df.selectExpr("format_number(value, format2)", "format_number(value, format1)").noop()
df.selectExpr("format_number(value, format2)").noop()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
runBenchmark("format_number") {
val benchmark = new Benchmark("format_number", N, output = output)
benchmark.addCase("optimize", M) { _ =>
doBenchmark()
}
benchmark.run()
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2448805047
> @panbingkun I think the benchmark is useful in catching regressions in the expression. Could you add it in?
Allow me to do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2447635050
@panbingkun I think the benchmark is useful in catching regressions in the expression. Could you add it in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2448900829
> @panbingkun I think the benchmark is useful in catching regressions in the expression. Could you add it in?
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #48702: [SPARK-50169][SQL] Improve performance of `RegExpReplace`
URL: https://github.com/apache/spark/pull/48702
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823893660
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
I noticed another function `FormatNumber`, but according to the results of my local benchmark test, I did not see great performance gain (`almost no change`).
https://github.com/apache/spark/blob/beea9c36725e7aa8f17f952da70e1d25013dc336/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L3345
```scala
object FormatNumberBenchmark extends SqlBasedBenchmark {
private val N = 1000000 // 10_000_00
private val M = 100
private val df = spark.range(N).to(new StructType().add("id", "int")).
withColumn("format1", lit("##################.##")).
withColumn("format2", lit(4)).
withColumn("value", col("id") / M)
private def doBenchmark(): Unit = {
// df.selectExpr("format_number(value, format2)", "format_number(value, format1)").noop()
df.selectExpr("format_number(value, format2)").noop()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
runBenchmark("format_number") {
val benchmark = new Benchmark("format_number", N, output = output)
benchmark.addCase("optimize", M) { _ =>
doBenchmark()
}
benchmark.run()
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2449166444
Thanks all @MaxGekk @dongjoon-hyun @yaooqinn @HyukjinKwon @zhengruifeng @zeal-thinker ❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1823893660
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
I noticed another function `FormatNumber`, but unfortunately, according to the results of my local benchmark test, I did not see great performance gain (`almost no change`).
https://github.com/apache/spark/blob/beea9c36725e7aa8f17f952da70e1d25013dc336/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L3345
```scala
object FormatNumberBenchmark extends SqlBasedBenchmark {
private val N = 1000000 // 10_000_00
private val M = 100
private val df = spark.range(N).to(new StructType().add("id", "int")).
withColumn("format1", lit("##################.##")).
withColumn("format2", lit(4)).
withColumn("value", col("id") / M)
private def doBenchmark(): Unit = {
// df.selectExpr("format_number(value, format2)", "format_number(value, format1)").noop()
df.selectExpr("format_number(value, format2)").noop()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
runBenchmark("format_number") {
val benchmark = new Benchmark("format_number", N, output = output)
benchmark.addCase("optimize", M) { _ =>
doBenchmark()
}
benchmark.run()
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "zeal-thinker (via GitHub)" <gi...@apache.org>.
zeal-thinker commented on code in PR #48702:
URL: https://github.com/apache/spark/pull/48702#discussion_r1824902514
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -681,7 +682,7 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
@transient private var lastReplacement: String = _
@transient private var lastReplacementInUTF8: UTF8String = _
// result buffer write by Matcher
- @transient private lazy val result: StringBuffer = new StringBuffer
+ @transient private lazy val result: JStringBuilder = new JStringBuilder
Review Comment:
All right! Thank you so much
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50169][SQL] Improve performance of `RegExpReplace` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #48702:
URL: https://github.com/apache/spark/pull/48702#issuecomment-2449129250
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org