-
Notifications
You must be signed in to change notification settings - Fork 1.7k
refactor: include metric output_batches into BaselineMetrics #18491
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
base: main
Are you sure you want to change the base?
refactor: include metric output_batches into BaselineMetrics #18491
Conversation
36ac5e0 to
744a4ae
Compare
744a4ae to
6272010
Compare
|
Hey @2010YOUY01, please do review once you get time. |
2010YOUY01
left a comment
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.
Thank you, this PR is well written.
This is good to go once the minor suggestions are addressed and tests pass.
|
@2010YOUY01, have done the requested changes, do check. |
|
It seems, with the https://github.com/apache/datafusion/actions/runs/19165362013/job/54785692452?pr=18491 |
|
@2010YOUY01, can you give it another go, updated the test and added baseline metrics in |
|
@2010YOUY01, it seems that #18262 will have proper baseline metrics for the Should we remove the basic |
2010YOUY01
left a comment
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.
LGTM, thanks again!
Should we remove the basic RepartitionExec BaselineMetrics I added here (from which we currently use the output_batches), or are we okay with having it, since once #18262 is merged, it should be fixed?
It's okay to keep it, we just merged a big change in RepartitionExec, so #18262 has to be reworked, unfortunately...
Which issue does this PR close?
output_batchesintoBaselineMetrics#17027Rationale for this change
output_batchesshould be a common metric in all operators, thus should ideally be added toBaselineMetricsWhat changes are included in this PR?
output_batchesintoBaselineMetricswithDEVMetricTyperecord_poll()APIoutput_bytesto baseline metrics #18268assert_metricsmacro to take multiple metrics strings for substring checkoutput_bytesandoutput_batchestracking inTopKoperatorbaselinemetrics forRepartitionExecAre these changes tested?
Added UT
Are there any user-facing changes?
Changes in the
EXPLAIN ANALYZEoutput,output_batcheswill be added tometrics=[...]