-
Notifications
You must be signed in to change notification settings - Fork 265
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
feat: add support for explain analyze #3484
Conversation
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
fde124f
to
d50c378
Compare
Here is an example of current output:
I think this gets us close but not quite there. I'm going to look at replacing the builtin "BaselineMetrics" with our own thing that captures some additional detail. My goal is to replicate what's available in postgres if possible:
I'll keep plugging on this but I think current state is ready for review. I need to add a test as well. One high-level question: |
Cache hit/miss rates would be awesome to incorporate here too. I don't know that I will get to that. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3484 +/- ##
==========================================
- Coverage 78.46% 78.45% -0.02%
==========================================
Files 252 252
Lines 93800 93917 +117
Branches 93800 93917 +117
==========================================
+ Hits 73604 73679 +75
- Misses 17201 17245 +44
+ Partials 2995 2993 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I like the framework and start.
I like the API and actually I kind of prefer two separate methods. In my mind they are quite different because analyze
is going to actually execute the query where explain
does not so I'm not sure how I feel about distinguishing with a flag.
Glad to see num_rows. 11s for filter compute is shocking but that does kind of match my gut of what I've been seeing in some contains queries.
Just a few nits about error handling. I'd prefer to make sure we are passing through child errors into parent error messages anytime we don't know exactly what the inner error is.
E.g. if the inner error is "table x did not exist" then it's fine to remap to "grabbing table x from database y did not exist" and drop the inner error. However, when doing a generic mapping to translate from one error type to another we generally have to assume the inner error is the "root cause" and that needs to be included somehow.
These are minor things though so marking approve and I trust your judgement with what you want to do here.
Parameters | ||
---------- | ||
verbose : bool, default False | ||
Use a verbose output format. |
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.
I wonder what the difference even is between non-verbose and verbose explain analyze and if it's worth giving users the choice? I know for explain_plan I have always passed verbose=True
and have never felt it is too verbose.
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.
the outputs look identical to me. Maybe it's some feature we're not using. I'll remove the option.
rust/lance/src/dataset/scanner.rs
Outdated
if let Ok(mut stream) = analyze.execute(0, ctx) { | ||
while (stream.next().await).is_some() {} | ||
} else { | ||
return Err(Error::Execution { | ||
message: "Failed to execute analyze plan".to_string(), | ||
location: location!(), | ||
}); | ||
} |
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.
Why not use map_err
and include the error message from analyze.execute
?
E.g. format!(Failed to execute analyze plan: {}
, err)`
rust/lance/src/io/exec/utils.rs
Outdated
std::task::Poll::Ready(Some(res)) => std::task::Poll::Ready(Some( | ||
res.map_err(|e| DataFusionError::External(e.to_string().into())), | ||
)), |
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.
Why do we need map_err
here? Isn't the error already a DataFusionError
?
In fact, do we even need this match statement at all? Can we just do let poll = this.stream.poll_next(cx);
?
This adds runtime execution metrics to all of our exec nodes. These metrics can be accessed by calling plan.analyze_plan().
c488bd6
to
a1d1abf
Compare
This adds runtime execution metrics to all of our exec nodes. These metrics can be accessed by calling plan.analyze_plan().