-
Notifications
You must be signed in to change notification settings - Fork 110
misc: Create expression benchmark for default engine #1220
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1220 +/- ##
==========================================
+ Coverage 83.81% 83.98% +0.16%
==========================================
Files 111 111
Lines 26223 26175 -48
Branches 26223 26175 -48
==========================================
+ Hits 21980 21983 +3
+ Misses 3147 3098 -49
+ Partials 1096 1094 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! one nit
kernel/benches/expression_bench.rs
Outdated
use delta_kernel::arrow::datatypes::{DataType, Field, Fields}; | ||
use delta_kernel::engine::arrow_expression::evaluate_expression::to_json; | ||
|
||
use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs for black_box:
👎Deprecated: use std::hint::black_box() instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh true. I didn't notice because we are using an older version of criterion
(from 2023) in which this wasn't deprecated yet. Should we upgrade?
0dc37b4
to
7cc435f
Compare
7cc435f
to
1cf0b1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, LGTM thanks!
## What changes are proposed in this pull request? delta-io#1192 added a makeshift benchmark test to measure the performance of the new `ToJson` expression. This PR migrates this test to a dedicated `expression_bench` for the default engine. This PR only adds `ToJson` to `expression_bench`, but this can be extended as necessary in future PRs. ## How was this change tested? N/A - this PR adds a performance benchmark. --------- Co-authored-by: Zach Schuermann <[email protected]>
What changes are proposed in this pull request?
#1192 added a makeshift benchmark test to measure the performance of the new
ToJson
expression. This PR migrates this test to a dedicatedexpression_bench
for the default engine.This PR only adds
ToJson
toexpression_bench
, but this can be extended as necessary in future PRs.How was this change tested?
N/A - this PR adds a performance benchmark.