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/16 08:05:56 UTC
[PR] [SPARK-49990][SQL] Improve performance of `randStr` [spark]
panbingkun opened a new pull request, #48493:
URL: https://github.com/apache/spark/pull/48493
### 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?
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48493:
URL: https://github.com/apache/spark/pull/48493#discussion_r1805910466
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -418,32 +418,33 @@ case class RandStr(
bytes.update(i, ('A' + num - 36).toByte)
}
}
- val result: UTF8String = UTF8String.fromBytes(bytes.toArray)
- result
+ UTF8String.fromBytes(bytes)
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val className = classOf[XORShiftRandom].getName
val rngTerm = ctx.addMutableState(className, "rng")
ctx.addPartitionInitializationStatement(
s"$rngTerm = new $className(${seed}L + partitionIndex);")
+ val bytesTerm = ctx.freshName("bytes")
+ val i = ctx.freshName("i")
+ val v = ctx.freshName("v")
val eval = length.genCode(ctx)
ev.copy(code =
code"""
|${eval.code}
- |int length = (int)(${eval.value});
- |char[] chars = new char[length];
- |for (int i = 0; i < length; i++) {
- | int v = Math.abs($rngTerm.nextInt() % 62);
- | if (v < 10) {
- | chars[i] = (char)('0' + v);
- | } else if (v < 36) {
- | chars[i] = (char)('a' + (v - 10));
+ |byte[] $bytesTerm = new byte[(int)(${eval.value})];
Review Comment:
It's almost very difficult to use `StaticInvoke/Invoke` + `RuntimeReplaceable` to unify it, because:
- We cannot have `RandStr` inherit both `Nondeterministic` and `RuntimeReplaceable` at the same time, they all define `eval()...` as `final` internally, and compilation will result in errors
https://github.com/apache/spark/blob/5697df70b3b7ffdd5e38c2099515cb73a57a79bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L510-L514
https://github.com/apache/spark/blob/5697df70b3b7ffdd5e38c2099515cb73a57a79bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L423-L424
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2421852776
> Can we rewrite `RandStr` to a new expression that extends `Nondeterministic`? We can use `InheritAnalysisRules` so that the `Nondeterministic` expression is still available in the expression tree.
Let me give it a try.
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48493: [SPARK-49990][SQL] Improve performance of `randStr`
URL: https://github.com/apache/spark/pull/48493
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2416042475
- Benchmark code as follows:
```scala
object RandstrBenchmark extends SqlBasedBenchmark {
private val N = 1000000 // 1_000_00
private val M = 100
private val df = spark.range(N).to(new StructType().add("id", "int"))
private def doBenchmark(): Unit = {
df.selectExpr("randStr(10, 20)").noop()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
runBenchmark("randStr") {
val benchmark = new Benchmark("randStr", N, output = output)
benchmark.addCase("optimize", M) { _ =>
doBenchmark()
}
benchmark.run()
}
}
}
```
- Before (run 3 times)
```
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 12265 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 107 123 21 9.4 106.9 1.0X
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 11298 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 107 113 7 9.3 107.0 1.0X
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 11181 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 107 112 5 9.4 106.7 1.0X
```
- After (run 3 times)
```
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 10362 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 98 104 6 10.2 98.0 1.0X
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 10380 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 98 104 7 10.2 97.9 1.0X
Running benchmark: randStr
Running case: optimize
Stopped after 100 iterations, 10230 ms
OpenJDK 64-Bit Server VM 17.0.10+7-LTS on Mac OS X 15.0.1
Apple M2
randStr: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
optimize 97 102 4 10.3 97.5 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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2421874872
actually, nvm. The issue is we must implement codegen manually for `Nondeterministic`. It won't make much difference if we use a smaller expression to extend `Nondeterministic`
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48493:
URL: https://github.com/apache/spark/pull/48493#discussion_r1805913291
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -418,32 +418,33 @@ case class RandStr(
bytes.update(i, ('A' + num - 36).toByte)
}
}
- val result: UTF8String = UTF8String.fromBytes(bytes.toArray)
- result
+ UTF8String.fromBytes(bytes)
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val className = classOf[XORShiftRandom].getName
val rngTerm = ctx.addMutableState(className, "rng")
ctx.addPartitionInitializationStatement(
s"$rngTerm = new $className(${seed}L + partitionIndex);")
+ val bytesTerm = ctx.freshName("bytes")
+ val i = ctx.freshName("i")
+ val v = ctx.freshName("v")
val eval = length.genCode(ctx)
ev.copy(code =
code"""
|${eval.code}
- |int length = (int)(${eval.value});
- |char[] chars = new char[length];
- |for (int i = 0; i < length; i++) {
- | int v = Math.abs($rngTerm.nextInt() % 62);
- | if (v < 10) {
- | chars[i] = (char)('0' + v);
- | } else if (v < 36) {
- | chars[i] = (char)('a' + (v - 10));
+ |byte[] $bytesTerm = new byte[(int)(${eval.value})];
Review Comment:
Currently, I have extracted the internal logic of `randstr` to `ExpressionImplUtils#randStr(...)` and called it separately in `interpreted` and `codegen`.
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48493:
URL: https://github.com/apache/spark/pull/48493#discussion_r1804373886
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -418,32 +418,33 @@ case class RandStr(
bytes.update(i, ('A' + num - 36).toByte)
}
}
- val result: UTF8String = UTF8String.fromBytes(bytes.toArray)
- result
+ UTF8String.fromBytes(bytes)
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val className = classOf[XORShiftRandom].getName
val rngTerm = ctx.addMutableState(className, "rng")
ctx.addPartitionInitializationStatement(
s"$rngTerm = new $className(${seed}L + partitionIndex);")
+ val bytesTerm = ctx.freshName("bytes")
+ val i = ctx.freshName("i")
+ val v = ctx.freshName("v")
val eval = length.genCode(ctx)
ev.copy(code =
code"""
|${eval.code}
- |int length = (int)(${eval.value});
- |char[] chars = new char[length];
- |for (int i = 0; i < length; i++) {
- | int v = Math.abs($rngTerm.nextInt() % 62);
- | if (v < 10) {
- | chars[i] = (char)('0' + v);
- | } else if (v < 36) {
- | chars[i] = (char)('a' + (v - 10));
+ |byte[] $bytesTerm = new byte[(int)(${eval.value})];
Review Comment:
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2418183545
cc @MaxGekk @cloud-fan
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2421818717
Can we rewrite `RandStr` to a new expression that extends `Nondeterministic`? We can use `InheritAnalysisRules` so that the `Nondeterministic` expression is still available in the expression tree.
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #48493:
URL: https://github.com/apache/spark/pull/48493#discussion_r1805910466
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -418,32 +418,33 @@ case class RandStr(
bytes.update(i, ('A' + num - 36).toByte)
}
}
- val result: UTF8String = UTF8String.fromBytes(bytes.toArray)
- result
+ UTF8String.fromBytes(bytes)
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val className = classOf[XORShiftRandom].getName
val rngTerm = ctx.addMutableState(className, "rng")
ctx.addPartitionInitializationStatement(
s"$rngTerm = new $className(${seed}L + partitionIndex);")
+ val bytesTerm = ctx.freshName("bytes")
+ val i = ctx.freshName("i")
+ val v = ctx.freshName("v")
val eval = length.genCode(ctx)
ev.copy(code =
code"""
|${eval.code}
- |int length = (int)(${eval.value});
- |char[] chars = new char[length];
- |for (int i = 0; i < length; i++) {
- | int v = Math.abs($rngTerm.nextInt() % 62);
- | if (v < 10) {
- | chars[i] = (char)('0' + v);
- | } else if (v < 36) {
- | chars[i] = (char)('a' + (v - 10));
+ |byte[] $bytesTerm = new byte[(int)(${eval.value})];
Review Comment:
It's almost very difficult to use `StaticInvoke/Invoke` + `RuntimeReplaceable` to unify it, because:
- We cannot have `RandStr` inherit both `Nondeterministic` and `RuntimeReplaceable` at the same time, they all define `eval()...` as `final` internally, and compilation will result in errors
https://github.com/apache/spark/blob/5697df70b3b7ffdd5e38c2099515cb73a57a79bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L510-L514
https://github.com/apache/spark/blob/5697df70b3b7ffdd5e38c2099515cb73a57a79bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L423-L424
- We cannot remove `Nondeterministic` either, as we rely on `Nondeterministic` for the initialization of `XORShiftRandom`
https://github.com/apache/spark/blob/5697df70b3b7ffdd5e38c2099515cb73a57a79bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala#L360-L362
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2421962137
> actually, nvm. The issue is we must implement codegen manually for `Nondeterministic`. It won't make much difference if we use a smaller expression to extend `Nondeterministic`
yep, that makes sense, I see now.
thanks!
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48493:
URL: https://github.com/apache/spark/pull/48493#issuecomment-2422426022
+1, LGTM. Merging to master.
Thank you, @panbingkun and @cloud-fan for review.
--
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-49990][SQL] Improve performance of `randStr` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48493:
URL: https://github.com/apache/spark/pull/48493#discussion_r1804332276
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -418,32 +418,33 @@ case class RandStr(
bytes.update(i, ('A' + num - 36).toByte)
}
}
- val result: UTF8String = UTF8String.fromBytes(bytes.toArray)
- result
+ UTF8String.fromBytes(bytes)
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val className = classOf[XORShiftRandom].getName
val rngTerm = ctx.addMutableState(className, "rng")
ctx.addPartitionInitializationStatement(
s"$rngTerm = new $className(${seed}L + partitionIndex);")
+ val bytesTerm = ctx.freshName("bytes")
+ val i = ctx.freshName("i")
+ val v = ctx.freshName("v")
val eval = length.genCode(ctx)
ev.copy(code =
code"""
|${eval.code}
- |int length = (int)(${eval.value});
- |char[] chars = new char[length];
- |for (int i = 0; i < length; i++) {
- | int v = Math.abs($rngTerm.nextInt() % 62);
- | if (v < 10) {
- | chars[i] = (char)('0' + v);
- | } else if (v < 36) {
- | chars[i] = (char)('a' + (v - 10));
+ |byte[] $bytesTerm = new byte[(int)(${eval.value})];
Review Comment:
is it possible to use `StaticInvoke`/`Invoke` to unify interpreted and codegen?
--
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