Skip to content
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

perf: Add criterion benchmark for aggregate expressions #948

Merged
merged 6 commits into from
Sep 18, 2024

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented Sep 18, 2024

Which issue does this PR close?

N/A

Rationale for this change

Add Criterion benchmark to compare DataFusion and Comet aggregate expressions.

aggregate/avg_decimal_datafusion
                        time:   [653.56 µs 657.57 µs 662.06 µs]
aggregate/avg_decimal_comet
                        time:   [1.0581 ms 1.0592 ms 1.0604 ms]
aggregate/sum_decimal_datafusion
                        time:   [695.51 µs 696.48 µs 697.60 µs]
aggregate/sum_decimal_comet
                        time:   [1.0218 ms 1.0230 ms 1.0242 ms]

What changes are included in this PR?

New benchmark

How are these changes tested?

Manually

@andygrove andygrove changed the title perf: Add criterion benchmark for SumDecimal aggregation perf: Add criterion benchmark for aggregation expressions Sep 18, 2024
@andygrove andygrove changed the title perf: Add criterion benchmark for aggregation expressions perf: Add criterion benchmark for aggregate expressions Sep 18, 2024

[[bench]]
name = "aggregate"
harness = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: shall we add an empty line to the end of the file?

Copy link
Contributor

@huaxingao huaxingao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

});

group.bench_function("sum_decimal_comet", |b| {
let comet_sum_decimal = Arc::new(AggregateUDF::new_from_impl(SumDecimal::new(
Copy link
Contributor

@kazuyukitanimura kazuyukitanimura Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to use blackbox to avoid the optimization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added, thanks

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.02%. Comparing base (4032129) to head (13312aa).
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #948      +/-   ##
============================================
- Coverage     34.15%   34.02%   -0.13%     
+ Complexity      881      876       -5     
============================================
  Files           112      112              
  Lines         43276    43276              
  Branches       9572     9572              
============================================
- Hits          14779    14725      -54     
- Misses        25478    25518      +40     
- Partials       3019     3033      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andygrove andygrove merged commit d5116a1 into apache:main Sep 18, 2024
74 checks passed
@andygrove andygrove deleted the agg-bench branch September 18, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants