-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Investigate performance of decimal math / aggregation #670
Comments
andygrove
changed the title
Investigate performance of decimal math
Investigate performance of decimal math / aggregation
Jul 16, 2024
microbenchmark results @ sf=1
|
Benchmark runs @ sf=100 suggest that reading decimal from parquet could potentially be a performance issue.
|
Thanks @andygrove updated to the correct one |
kazuyukitanimura
added a commit
that referenced
this issue
Jul 24, 2024
## Which issue does this PR close? Part of #679 and #670 Related #490 ## Rationale for this change For dictionary decimal vectors, it was unpacking even for Int and Long decimals that used more memory than necessary. ## What changes are included in this PR? Unpack only for Decimal 128 ## How are these changes tested? Existing test
5 tasks
This was referenced Jul 25, 2024
This was referenced Jul 26, 2024
Merged
After #741 there are still some issues with
|
After merging with latest main
|
kazuyukitanimura
added a commit
that referenced
this issue
Aug 1, 2024
…#741) ## Which issue does this PR close? Part of #670 ## Rationale for this change This PR improves the native execution performance on decimals with a small precision ## What changes are included in this PR? This PR changes not to promote decimal128 to decimal256 if the precisions are small enough ## How are these changes tested? Existing tests
This was referenced Aug 1, 2024
kazuyukitanimura
added a commit
that referenced
this issue
Aug 2, 2024
## Which issue does this PR close? Part of #679 and #670 ## Rationale for this change The improvement could be negligible in real use cases, but I see some improvements in micro benchmarks ## What changes are included in this PR? Optimizations in some bit functions ## How are these changes tested? Existing tests
We created many fixes. I think decimals are no longer issues. closing for now |
himadripal
pushed a commit
to himadripal/datafusion-comet
that referenced
this issue
Sep 7, 2024
## Which issue does this PR close? Part of apache#679 and apache#670 Related apache#490 ## Rationale for this change For dictionary decimal vectors, it was unpacking even for Int and Long decimals that used more memory than necessary. ## What changes are included in this PR? Unpack only for Decimal 128 ## How are these changes tested? Existing test (cherry picked from commit c1b7c7d)
himadripal
pushed a commit
to himadripal/datafusion-comet
that referenced
this issue
Sep 7, 2024
…apache#741) ## Which issue does this PR close? Part of apache#670 ## Rationale for this change This PR improves the native execution performance on decimals with a small precision ## What changes are included in this PR? This PR changes not to promote decimal128 to decimal256 if the precisions are small enough ## How are these changes tested? Existing tests (cherry picked from commit 25957dd)
himadripal
pushed a commit
to himadripal/datafusion-comet
that referenced
this issue
Sep 7, 2024
## Which issue does this PR close? Part of apache#679 and apache#670 ## Rationale for this change The improvement could be negligible in real use cases, but I see some improvements in micro benchmarks ## What changes are included in this PR? Optimizations in some bit functions ## How are these changes tested? Existing tests (cherry picked from commit ffb96c3)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is the problem the feature request solves?
SQL
Query time in seconds with Comet disabled:
With Comet enabled:
I do not see a slow down if I add all the integer columns in the table, so this seems specific to decimal.
Part of the issue here may be the creation of all of the intermediate result vectors.
Describe the potential solution
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: