You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/12 04:26:40 UTC
[GitHub] [spark] sadikovi opened a new pull request, #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
sadikovi opened a new pull request, #37485:
URL: https://github.com/apache/spark/pull/37485
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR is a follow-up for https://github.com/apache/spark/pull/37293. The patch proposes to use `hasArray()` API to check whether or not the `ByteBuffer` has an underlying array that could be leveraged to direct access.
If the condition is not met, the code falls back to the original way of handling things using `ByteBuffer`s.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Fixes a potential issue of using direct byte buffers in Parquet-MR and makes the code forward-compatible.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No. This is an internal change.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Existing unit tests. I reran `DataSourceReadBenchmark` to ensure this `if-else` does not have a significant impact on performance.
--
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
[GitHub] [spark] sadikovi commented on pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1213509212
Thanks. @LuciferYang Do you have internal workloads that could be run with this patch to check if the performance is still on-par? I am a bit skeptical about the benchmarking results 🙂. 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
[GitHub] [spark] sadikovi commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944113337
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
How do you mean? Do you mean that there is no unit test to check? I can add a test that covers both cases.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944115044
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
I mean line 307 should not be covered by the UT now
--
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
[GitHub] [spark] sadikovi commented on a diff in pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944776977
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetDeltaEncodingSuite.scala:
##########
@@ -231,19 +230,33 @@ abstract class ParquetDeltaEncodingSuite[T] extends ParquetCompatibilityTest
}
private def shouldReadAndWrite(data: Array[T], length: Int): Unit = {
- writeData(data, length)
- reader = new VectorizedDeltaBinaryPackedReader
- val page = writer.getBytes.toByteArray
+ // SPARK-40052: Check that we can handle direct and non-direct byte buffers depending on the
+ // implementation of ByteBufferInputStream.
+ for (useDirect <- Seq(true, false)) {
+ writeData(data, length)
+ reader = new VectorizedDeltaBinaryPackedReader
+ val page = writer.getBytes.toByteArray
+
+ assert(estimatedSize(length) >= page.length)
+ writableColumnVector = new OnHeapColumnVector(data.length, getSparkSqlType)
+
+ val buf = if (useDirect) {
+ ByteBuffer.allocateDirect(page.length)
+ } else {
+ ByteBuffer.allocate(page.length)
+ }
+ buf.put(page)
+ buf.rewind()
Review Comment:
In this case, it is probably equivalent as the capacity is set to the the page length, but yeah, I can update.
--
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
[GitHub] [spark] sadikovi commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944106161
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
Review Comment:
Unfortunately, yes. ByteBufferInputStream does not have an API to check if it is backed by direct byte buffers. Let me check if I can have the boolean flag to be resolved once on the initialisation.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944109872
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
nit: should we add a tips or todo due to UT can't cover this branch now, I think we should test it in the future
--
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
[GitHub] [spark] sunchao closed pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sunchao closed pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
URL: https://github.com/apache/spark/pull/37485
--
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
[GitHub] [spark] LuciferYang commented on pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1212723009
@sadikovi I also use `DataSourceReadBenchmark `
--
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
[GitHub] [spark] sunchao commented on pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sunchao commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1213545192
Committed to master, thanks @sadikovi , @LuciferYang , @dongjoon-hyun !
--
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
[GitHub] [spark] sadikovi commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944115806
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
Yes, there are no unit tests for this. Even before this patch, this would only be covered if the encoding is enabled and data pages v2 are used.
--
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
[GitHub] [spark] sadikovi commented on pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1212720022
These are all good points. I am running the benchmarks and have updated the PR description with preliminary results. Interestingly, the results with `if` statement are slightly better which means there is a variability with the benchmark.
@LuciferYang Do you have a specific test case that I can run to compare performance?
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1213487998
To @sunchao , I believe I need your final look at the last commit. Please merge after your final 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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944115044
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
I mean line 307 is not covered by UT now
--
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
[GitHub] [spark] sadikovi commented on pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1212765192
I reran the benchmarks again, on a larger 4x dataset (I changed the size in DataSourceReadBenchmark). The numbers are still very similar with the patch performing slightly better than the current code. I don't quite understand how that is possible unless the benchmark does not exercise the encoding.
### Before
```
OpenJDK 64-Bit Server VM 1.8.0_312-8u312-b07-0ubuntu1~18.04-b07 on Linux 5.4.0-1071-aws
Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
Parquet Reader Single INT Column Scan: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
---------------------------------------------------------------------------------------------------------------------------
ParquetReader Vectorized: DataPageV1 672 707 45 93.6 10.7 1.0X
ParquetReader Vectorized: DataPageV2 945 1012 95 66.6 15.0 0.7X
ParquetReader Vectorized -> Row: DataPageV1 383 432 28 164.4 6.1 1.8X
ParquetReader Vectorized -> Row: DataPageV2 670 678 8 93.9 10.6 1.0X
OpenJDK 64-Bit Server VM 1.8.0_312-8u312-b07-0ubuntu1~18.04-b07 on Linux 5.4.0-1071-aws
Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
Parquet Reader Single BIGINT Column Scan: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
---------------------------------------------------------------------------------------------------------------------------
ParquetReader Vectorized: DataPageV1 931 935 4 67.6 14.8 1.0X
ParquetReader Vectorized: DataPageV2 1475 1477 4 42.7 23.4 0.6X
ParquetReader Vectorized -> Row: DataPageV1 638 650 14 98.5 10.1 1.5X
ParquetReader Vectorized -> Row: DataPageV2 1172 1173 2 53.7 18.6 0.8X
```
### After
```
[info] OpenJDK 64-Bit Server VM 1.8.0_312-8u312-b07-0ubuntu1~18.04-b07 on Linux 5.4.0-1071-aws
[info] Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
[info] Parquet Reader Single INT Column Scan: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ---------------------------------------------------------------------------------------------------------------------------
[info] ParquetReader Vectorized: DataPageV1 656 704 60 95.9 10.4 1.0X
[info] ParquetReader Vectorized: DataPageV2 888 898 12 70.9 14.1 0.7X
[info] ParquetReader Vectorized -> Row: DataPageV1 393 435 24 160.2 6.2 1.7X
[info] ParquetReader Vectorized -> Row: DataPageV2 667 681 12 94.3 10.6 1.0X
[info] OpenJDK 64-Bit Server VM 1.8.0_312-8u312-b07-0ubuntu1~18.04-b07 on Linux 5.4.0-1071-aws
[info] Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
[info] Parquet Reader Single BIGINT Column Scan: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ---------------------------------------------------------------------------------------------------------------------------
[info] ParquetReader Vectorized: DataPageV1 935 953 16 67.3 14.9 1.0X
[info] ParquetReader Vectorized: DataPageV2 1437 1440 4 43.8 22.8 0.7X
[info] ParquetReader Vectorized -> Row: DataPageV1 717 731 12 87.7 11.4 1.3X
[info] ParquetReader Vectorized -> Row: DataPageV2 1176 1185 13 53.5 18.7 0.8X
```
--
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
[GitHub] [spark] sunchao commented on a diff in pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sunchao commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944160446
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetDeltaEncodingSuite.scala:
##########
@@ -231,19 +230,33 @@ abstract class ParquetDeltaEncodingSuite[T] extends ParquetCompatibilityTest
}
private def shouldReadAndWrite(data: Array[T], length: Int): Unit = {
- writeData(data, length)
- reader = new VectorizedDeltaBinaryPackedReader
- val page = writer.getBytes.toByteArray
+ // SPARK-40052: Check that we can handle direct and non-direct byte buffers depending on the
+ // implementation of ByteBufferInputStream.
+ for (useDirect <- Seq(true, false)) {
+ writeData(data, length)
+ reader = new VectorizedDeltaBinaryPackedReader
+ val page = writer.getBytes.toByteArray
+
+ assert(estimatedSize(length) >= page.length)
+ writableColumnVector = new OnHeapColumnVector(data.length, getSparkSqlType)
+
+ val buf = if (useDirect) {
+ ByteBuffer.allocateDirect(page.length)
+ } else {
+ ByteBuffer.allocate(page.length)
+ }
+ buf.put(page)
+ buf.rewind()
Review Comment:
should we call `buf.flip()` instead?
--
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
[GitHub] [spark] LuciferYang commented on pull request #37485: [SPARK-40052][SQL] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1212788480
I believe that the `unpack8Values` method needs to be executed for the Int and Long type data of Parquet v2. For the two cases we are concerned(15m data), it will be executed more than 50000 times :)
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37485:
URL: https://github.com/apache/spark/pull/37485#issuecomment-1212720303
Thank you for the detail in the PR description. I saw it now, @sadikovi .
--
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
[GitHub] [spark] sadikovi commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
sadikovi commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944115806
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
+ packer.unpack8Values(buffer.array(),
+ buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ } else {
+ packer.unpack8Values(buffer, buffer.position(), unpackedValuesBuffer, j);
Review Comment:
Yes, there are no unit tests for this. Even before this patch, this would only be covered if the encoding is enabled and data pages v2 are used and the original PR did not have unit tests for array usage.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37485: [SPARK-40052] Handle direct byte buffers in VectorizedDeltaBinaryPackedReader
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37485:
URL: https://github.com/apache/spark/pull/37485#discussion_r944105142
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaBinaryPackedReader.java:
##########
@@ -300,8 +300,12 @@ private void unpackMiniBlock() throws IOException {
bitWidths[currentMiniBlock]);
for (int j = 0; j < miniBlockSizeInValues; j += 8) {
ByteBuffer buffer = in.slice(packer.getBitWidth());
- packer.unpack8Values(buffer.array(),
- buffer.arrayOffset() + buffer.position(), unpackedValuesBuffer, j);
+ if (buffer.hasArray()) {
Review Comment:
This is definitely right, but do we need to make a check on each `buffer`?
--
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