You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "wankunde (via GitHub)" <gi...@apache.org> on 2023/12/14 08:36:35 UTC
[PR] [SPARK-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
wankunde opened a new pull request, #44351:
URL: https://github.com/apache/spark/pull/44351
### What changes were proposed in this pull request?
Now spark will get a parquet binary object with getBytes() method.
The **Binary.getBytes()** method will always make a new copy of the internal bytes.
We can use **Binary.getBytesUnsafe()** method to get the cached bytes if it has already been called getBytes() and has the cached bytes.
Local benchmark, before this PR:
```
OpenJDK 64-Bit Server VM 17.0.6+10-LTS on Mac OS X 13.6
Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Parquet dictionary: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Read binary dictionary 18919 19449 393 5.5 180.4 1.0X
```
after this PR:
```
OpenJDK 64-Bit Server VM 17.0.6+10-LTS on Mac OS X 13.6
Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Parquet dictionary: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Read binary dictionary 10135 10602 337 10.3 96.7 1.0X
```
### Why are the changes needed?
Optimize parquet reader.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Local test
### 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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "wankunde (via GitHub)" <gi...@apache.org>.
wankunde commented on PR #44351:
URL: https://github.com/apache/spark/pull/44351#issuecomment-1855412209
Local benchmark code
```scala
object ParquetDictionaryDecoderBenchmark extends SqlBasedBenchmark {
import spark.implicits._
private val N = 100 * 1024 * 1024
private val NUMBER_OF_ITER = 10
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
withTempPath { tempDir =>
val outputPath = tempDir.getCanonicalPath
val df: DataFrame = spark
.range(1, N, 1, 1)
.map(id => ("ABCDEFG", "HIJKLMN", "OPQRST", "UVWXYZ"))
.toDF("a", "b", "c", "d")
df.write.mode(SaveMode.Overwrite).parquet(outputPath)
val benchmark = new Benchmark("Parquet dictionary", N, NUMBER_OF_ITER, output = output)
benchmark.addCase("Read binary dictionary") { _ =>
spark.read.parquet(outputPath).noop()
}
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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44351:
URL: https://github.com/apache/spark/pull/44351#discussion_r1426639813
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetDictionary.java:
##########
@@ -70,7 +70,7 @@ public byte[] decodeToBinary(int id) {
long signed = dictionary.decodeToLong(id);
return new BigInteger(Long.toUnsignedString(signed)).toByteArray();
} else {
- return dictionary.decodeToBinary(id).getBytes();
+ return dictionary.decodeToBinary(id).getBytesUnsafe();
Review Comment:
Are you sure it is safe?
--
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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #44351:
URL: https://github.com/apache/spark/pull/44351#issuecomment-1857684317
also cc @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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer closed pull request #44351: [SPARK-46403][SQL] Decode parquet binary with getBytesUnsafe method
URL: https://github.com/apache/spark/pull/44351
--
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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "wankunde (via GitHub)" <gi...@apache.org>.
wankunde commented on code in PR #44351:
URL: https://github.com/apache/spark/pull/44351#discussion_r1426902797
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetDictionary.java:
##########
@@ -70,7 +70,7 @@ public byte[] decodeToBinary(int id) {
long signed = dictionary.decodeToLong(id);
return new BigInteger(Long.toUnsignedString(signed)).toByteArray();
} else {
- return dictionary.decodeToBinary(id).getBytes();
+ return dictionary.decodeToBinary(id).getBytesUnsafe();
Review Comment:
The comment of `getBytesUnsafe()` method:
```
/**
* Variant of getBytes() that avoids copying backing data structure by returning
* backing byte[] of the Binary. Do not modify backing byte[] unless you know what
* you are doing.
*
* @return backing byte[] of correct size, with an offset of 0, if possible, else returns result of getBytes()
*/
public abstract byte[] getBytesUnsafe();
```
Spark will copy those cached bytes before using them, so I think it's safe for spark.
--
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-46403][SQL] Decode parquet binary with getBytesUnsafe method [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44351:
URL: https://github.com/apache/spark/pull/44351#issuecomment-1858686712
Merged to master
@wankunde @yaooqinn @cloud-fan Thank you.
--
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