-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ui,sql: add Vectorized info to statements page #50255
Conversation
Thanks for jumping on this! Do you have a screenshot of what this will look like? |
Thanks for sharing! What does 1 of 2 mean? |
It is the same as for |
Gotcha. I predict that it will be confusing to the user why the query fingerprint is sometimes run in vectorized and sometime not. In fact, why would that be the case? Is this an indication that we shouldn't actually be grouping these two queries under the same fingerprint? |
Yeah, it might be confusing, but we use the exactly same representation for "Distributed execution" (as well as for "Used cost-based optimizer" but this one is now always
In my scenario I explicitly changed
I think the current grouping is reasonable, and I don't have a good feeling of what would be more preferable grouping criteria. |
Alright, I opened up #50284 to discuss this further--don't let that conversation impact this PR. |
a26e3f4
to
310516a
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.
LGTM (regarding UI changes)
310516a
to
86907fb
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.
stats related changes look fine
pkg/sql/app_stats.go
Outdated
@@ -141,7 +143,7 @@ func (a *appStats) recordStatement( | |||
} | |||
|
|||
// Get the statistics object. | |||
s := a.getStatsForStmt(stmt, distSQLUsed, implicitTxn, err, true /* createIfNonexistent */) | |||
s := a.getStatsForStmt(stmt, distSQLUsed, vectorized, implicitTxn, err, true /* createIfNonexistent */) |
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.
[nit]: this is too long for one line now
pkg/sql/distsql_running.go
Outdated
} | ||
|
||
return ctx, flow, nil | ||
return ctx, flow, setupReq.EvalContext.Vectorize != int32(sessiondata.VectorizeOff), nil |
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 don't we check here if the flow was vectorized or not? Why do we look at a setting
This commit adds a box to Statements page that displays whether the vectorized execution engine is used. Release note (admin ui change): Statements page now contains information about which execution engine is used (vectorized or not).
86907fb
to
c77d5f0
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rohany)
pkg/sql/app_stats.go, line 146 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
[nit]: this is too long for one line now
Done.
pkg/sql/distsql_running.go, line 267 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
why don't we check here if the flow was vectorized or not? Why do we look at a setting
Indeed, good point, fixed.
TFTRs! bors r+ |
Build succeeded |
This commit adds a box to Statements page that displays whether the
vectorized execution engine is used.
Release note (admin ui change): Statements page now contains information
about which execution engine is used (vectorized or not).