-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Improve SQLMetric APIs, port existing metrics #908
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
|
Here is an example of what is needed to use the new API in IOx: https://github.com/influxdata/influxdb_iox/pull/2385 |
alamb
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.
@andygrove @Dandandan @houqp I am sorry for the large PR -- though it is mostly doc comments / tests.
This PR incorporates the first round of feedback from @tustvold and @andygrove
on #901)
FYI @returnString (who wrote the initial metrics API)
| /// that had the same name and partition=`Some(..)` have been | ||
| /// aggregated together. The resulting `MetricsSet` has all | ||
| /// metrics with `Partition=None` | ||
| pub fn aggregate_by_partition(&self) -> Self { |
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.
Here is the code that can aggregate metrics by partition (and sum above is also useful)
andygrove
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.
This looks very good. Thanks @alamb
|
Thanks @andygrove -- I plan to merge this in later today unless I anyone objects or would like more time to review. |
returnString
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.
This looks awesome, huge usability improvement 👍
| let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap(); | ||
| let formatted = normalize_for_explain(&formatted); | ||
|
|
||
| println!("ANALYZE EXPLAIN:\n{}", formatted); |
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.
Stray debug logging?
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.
Really it is more like a development tool. I will plan to remove it as a follow on PR.
|
For anyone else who is updating code that uses the existing |
|
Late to the party, this is really great work, thanks @alamb ! |
Which issue does this PR close?
Closes #679. See #901 for an earlier version with feedback from @tustvold and @andygrove
Rationale for this change
See the description on #679 (comment) for the full rationale, but the TLDR version is:
What changes are included in this PR?
SQLMetricAPI to be in its own module, have labels, know about partitions, and allow for real time inspectionSQLMetric-->Metricoutput_rows) rather than camel 🐫 case (outputRows) to conform to Rust expectations (see note from @andygrove on the reason the names were camelCase to begin with: Implement new metrics API / RFC #901 (comment))Are there any user-facing changes?
YES!
The
SQLMetric/MetricAPI is now totally different so any code that creates / usesSQLMetricswould have to be updated. The updates are fairly mechanical as you can see in this PR)Notes
In keeping with Rust's tradition of static typing, I also changed to using more strongly typed versions of the metric values to avoid mistakes such as adding a "time" to a counter value, as well as allowing other counter specific operations.
Also, as the metrics aren't specific to SQL (they apply to any ExecutionPlan, even if that plan was created via the DataFrame API or the LogicalPlanBuilder) I renamed
SQLMetric-->MetricNot included in this PR: