You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mingmwang (via GitHub)" <gi...@apache.org> on 2023/04/04 16:03:32 UTC
[GitHub] [arrow-datafusion] mingmwang opened a new pull request, #5866: Improve avg/sum Aggregator performance
mingmwang opened a new pull request, #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5858 #5859.
# Rationale for this change
Improve avg(decimal) and sum(decimal) performance, avoid type cast in the inner loop, improve the TPCH-q17 performance
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496525305
Although PR shows an improvement, ~15% of the CPU samples still end up in `cast`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496288024
Great work, I can take a look tomorrow and rerun the benchmarks on my machine.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1498602704
> Here are the results on my desktop (24 core) for queries 17 and 18 (sf=10)
>
> ## This PR
> ```
> Executing query 17 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q17.sql
> Query 17 executed in: 25.076557605s
> Query 17 executed in: 14.468542149s
> Query 17 executed in: 13.238961128s
> Executing query 18 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q18.sql
> Query 18 executed in: 8.197590942s
> Query 18 executed in: 7.883707736s
> Query 18 executed in: 7.936216688s
> ```
>
> ## Main branch
> ```
> Executing query 17 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q17.sql
> Query 17 executed in: 20.10441009s
> Query 17 executed in: 14.731355792s
> Query 17 executed in: 13.869329179s
> Executing query 18 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q18.sql
> Query 18 executed in: 8.900509355s
> Query 18 executed in: 8.695394752s
> Query 18 executed in: 8.541570444s
> Executing query 19 from /home/andy/git
> ```
Could you please rerun the benchmark with partition = 1 or partition = 2 ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on a diff in pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on code in PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#discussion_r1160604767
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -31,3 +33,49 @@ pub fn get_accum_scalar_values_as_arrays(
.map(|s| s.to_array_of_size(1))
.collect::<Vec<_>>())
}
+
+pub fn calculate_result_decimal_for_avg(
Review Comment:
I think it is a little bit different then the explicit arithmetic expression in the SQL, the reason is this final result decimal conversion is implicit, the logical plans/optimizers actually are not aware of this implicit conversion. This is similar to the implicit `cast` that I pulled up this PR, the cast is also implicit and the logical plans/optimizers are not aware of this implicit `cast` either. Those implicit `cast` and conversions are only related to internal state of some specific Aggregation `Accumulator`, they are not part of the SQL plans/expressions 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496387949
I tried testing the changes in this PR and ran into some errors when running query 1 using the code in https://github.com/sql-benchmarks/sqlbench-runners/tree/main/datafusion
```
thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
```
I don't see these errors when running against the latest in the main branch.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1498601825
> I tried testing the changes in this PR and ran into some errors when running query 1 using the code in https://github.com/sql-benchmarks/sqlbench-runners/tree/main/datafusion
>
> ```
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> ```
>
> I don't see these errors when running against the latest in the main branch.
I added the overflow check when converting the internal sum type to the result type, there was bug it is fixed 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yahoNanJing commented on pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "yahoNanJing (via GitHub)" <gi...@apache.org>.
yahoNanJing commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1501539486
LGTM.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5866: Improve avg/sum Aggregator performance
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#discussion_r1160154953
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -31,3 +33,49 @@ pub fn get_accum_scalar_values_as_arrays(
.map(|s| s.to_array_of_size(1))
.collect::<Vec<_>>())
}
+
+pub fn calculate_result_decimal_for_avg(
Review Comment:
This seems somewhat related to https://github.com/apache/arrow-datafusion/pull/5675 from @viirya to calculate the output size of a decimal multiplication. I wonder if there is some more general function / approach than special casing in the aggregator
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496291044
@mingmwang query 18 also is affected by this (although should be less of a difference than query 17)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1497884111
I run the test with sf=1 and partition = 1. I will do more test with sf= 10 tomorrow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496511876
When running in memory (SF=1)
```
cargo run --release --bin tpch benchmark datafusion --path ./data/ --format parquet --partitions 16 -q 17 --iterations 10 -m
```
main
==
```
Query 17 iteration 0 took 725.6 ms and returned 1 rows
Query 17 iteration 1 took 675.3 ms and returned 1 rows
Query 17 iteration 2 took 635.1 ms and returned 1 rows
Query 17 iteration 3 took 702.1 ms and returned 1 rows
Query 17 iteration 4 took 624.7 ms and returned 1 rows
Query 17 iteration 5 took 692.2 ms and returned 1 rows
Query 17 iteration 6 took 681.0 ms and returned 1 rows
Query 17 iteration 7 took 668.9 ms and returned 1 rows
Query 17 iteration 8 took 675.6 ms and returned 1 rows
Query 17 iteration 9 took 660.6 ms and returned 1 rows
Query 17 avg time: 674.12 ms
```
```
Query 18 iteration 0 took 377.6 ms and returned 57 rows
Query 18 iteration 1 took 361.1 ms and returned 57 rows
Query 18 iteration 2 took 353.7 ms and returned 57 rows
Query 18 iteration 3 took 362.2 ms and returned 57 rows
Query 18 iteration 4 took 359.8 ms and returned 57 rows
Query 18 iteration 5 took 356.8 ms and returned 57 rows
Query 18 iteration 6 took 350.8 ms and returned 57 rows
Query 18 iteration 7 took 360.6 ms and returned 57 rows
Query 18 iteration 8 took 351.6 ms and returned 57 rows
Query 18 iteration 9 took 352.2 ms and returned 57 rows
Query 18 avg time: 358.65 ms
```
PR
==
```
Query 17 iteration 0 took 640.5 ms and returned 1 rows
Query 17 iteration 1 took 649.0 ms and returned 1 rows
Query 17 iteration 2 took 597.3 ms and returned 1 rows
Query 17 iteration 3 took 639.3 ms and returned 1 rows
Query 17 iteration 4 took 645.2 ms and returned 1 rows
Query 17 iteration 5 took 653.8 ms and returned 1 rows
Query 17 iteration 6 took 628.6 ms and returned 1 rows
Query 17 iteration 7 took 643.7 ms and returned 1 rows
Query 17 iteration 8 took 600.0 ms and returned 1 rows
Query 17 iteration 9 took 591.8 ms and returned 1 rows
Query 17 avg time: 628.91 ms
```
```
Query 18 iteration 0 took 355.8 ms and returned 57 rows
Query 18 iteration 1 took 333.5 ms and returned 57 rows
Query 18 iteration 2 took 317.5 ms and returned 57 rows
Query 18 iteration 3 took 327.6 ms and returned 57 rows
Query 18 iteration 4 took 318.2 ms and returned 57 rows
Query 18 iteration 5 took 324.2 ms and returned 57 rows
Query 18 iteration 6 took 315.0 ms and returned 57 rows
Query 18 iteration 7 took 323.1 ms and returned 57 rows
Query 18 iteration 8 took 314.8 ms and returned 57 rows
Query 18 iteration 9 took 322.0 ms and returned 57 rows
Query 18 avg time: 325.17 ms
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1499125555
> > I tried testing the changes in this PR and ran into some errors when running query 1 using the code in https://github.com/sql-benchmarks/sqlbench-runners/tree/main/datafusion
> > ```
> > thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> > thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> > thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I don't see these errors when running against the latest in the main branch.
>
> I can not reproduce the issue using DataFusion's own benchmark data(sf=10), but I'm able to reproduce the issue using Spark generated benchmark data. I guess Spark's tpch data schema is different with DataFusion's.
Maybe decimals vs floats? Official TPC-H uses decimals.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1498565574
> I tried testing the changes in this PR and ran into some errors when running query 1 using the code in https://github.com/sql-benchmarks/sqlbench-runners/tree/main/datafusion
>
> ```
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> thread 'tokio-runtime-worker' panicked at 'Unexpected accumulator state in hash aggregate: Internal("Arithmetic Overflow in AvgAccumulator")', /home/andy/.cargo/git/checkouts/arrow-datafusion-bfd9a8de51c58474/4e6eac5/datafusion/core/src/physical_plan/aggregates/row_hash.rs:642:81
> ```
>
> I don't see these errors when running against the latest in the main branch.
I can not reproduce the issue using DataFusion's own benchmark data(sf=10), but I'm able to reproduce the issue using Spark generated benchmark data. I guess Spark's tpch data schema is different with DataFusion's.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yahoNanJing merged pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "yahoNanJing (via GitHub)" <gi...@apache.org>.
yahoNanJing merged PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yahoNanJing commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "yahoNanJing (via GitHub)" <gi...@apache.org>.
yahoNanJing commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1498425875
@Dandandan, how did you generate the flame graph? At my side, there're many unknowns. Could you teach me how to avoid them.
`
cargo run --release --bin tpch -- benchmark datafusion --iterations 10 --path ./data-parquet --format parquet --partitions 1 --query 17
`
`sudo perf record -F 99 -g -p 917304`
`sudo perf script | ./stackcollapse-perf.pl | ./flamegraph.pl > perf.svg`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496408396
Here are the results on my desktop (24 core) for queries 17 and 18.
## This PR
```
Executing query 17 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q17.sql
Query 17 executed in: 25.076557605s
Query 17 executed in: 14.468542149s
Query 17 executed in: 13.238961128s
Executing query 18 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q18.sql
Query 18 executed in: 8.197590942s
Query 18 executed in: 7.883707736s
Query 18 executed in: 7.936216688s
```
## Main branch
```
Executing query 17 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q17.sql
Query 17 executed in: 20.10441009s
Query 17 executed in: 14.731355792s
Query 17 executed in: 13.869329179s
Executing query 18 from /home/andy/git/sql-benchmarks/sqlbench-h/queries/sf=10//q18.sql
Query 18 executed in: 8.900509355s
Query 18 executed in: 8.695394752s
Query 18 executed in: 8.541570444s
Executing query 19 from /home/andy/git
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1501157467
@Dandandan @andygrove @andygrove @yahoNanJing
Would you please help to review and approve this PR ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1499163653
> > >
> > > I don't see these errors when running against the latest in the main branch.
> >
> >
> > I can not reproduce the issue using DataFusion's own benchmark data(sf=10), but I'm able to reproduce the issue using Spark generated benchmark data. I guess Spark's tpch data schema is different with DataFusion's.
>
> Maybe decimals vs floats? Official TPC-H uses decimals.
Both are decimals, just the precision is a little different. Anyway, it is my bug.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yahoNanJing commented on pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "yahoNanJing (via GitHub)" <gi...@apache.org>.
yahoNanJing commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1502623259
Hi @Dandandan, @andygrove, @alamb, do you still have any concerns for this PR? At my side, this PR can achieve around 20% performance improvement for q17 with single partition. It would be better to merge this PR first and then continue other bottleneck refining :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yahoNanJing commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "yahoNanJing (via GitHub)" <gi...@apache.org>.
yahoNanJing commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1498587761
Hi @Dandandan, for the latest code, the **_cast_** is almost avoided. The related flame graphs are as follows:
With single partition benchmark, the latency is reduced from 11s to 9s for q17.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on a diff in pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on code in PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#discussion_r1160604767
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -31,3 +33,49 @@ pub fn get_accum_scalar_values_as_arrays(
.map(|s| s.to_array_of_size(1))
.collect::<Vec<_>>())
}
+
+pub fn calculate_result_decimal_for_avg(
Review Comment:
I think it is a little bit different then the explicit arithmetic expression in the SQL, the reason is this final result decimal conversion is implicit, the logical plans/optimizers actually are not aware of this implicit conversion. This is similar to the implicit `cast` that I pulled up this PR, the cast is also implicit and the logical plans/optimizers is not aware of this implicit `cast` either. Those implicit `cast` and conversion is only related to internal state of some specific Aggregation `Accumulator`, that are not part of the SQL plans/expressions 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance for Decimal
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1500214767
But in general, we can have a more general decimal precision change and overflow check logic for Decimal. And the Aggregators can reuse the logic.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1499341733
Hi @mingmwang , for generating the flamegraphs I currently use `cargo flamegraph` from https://github.com/flamegraph-rs/flamegraph .
I use the following command:
`CARGO_PROFILE_RELEASE_DEBUG=true cargo flamegraph --freq 5000 --bin tpch -- benchmark datafusion --path ./data/ --format parquet --partitions 16 -q 17 -d --iterations 10`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #5866: Improve avg/sum Aggregator performance
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5866:
URL: https://github.com/apache/arrow-datafusion/pull/5866#issuecomment-1496244903
@liukun4515 @yahoNanJing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org