colexec: clean up vectorized stats #53153
Conversation
|
This change is |
yuzefovich
force-pushed
the
cleanup-stats
branch
from
eabd520 to
b7bbd91
Compare
There was a problem hiding this comment.
Reviewed 5 of 10 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)
pkg/sql/colexec/execpb/stats.go, line 54 at r1 (raw file):
batchesOutputQueryPlanSuffix = "batches output" tuplesOutputQueryPlanSuffix = "tuples output" ioIncludedTimeQueryPlanSuffix = "IO + execution time"
Are you sure it includes execution time? I'm under the impression it's only IO.
pkg/sql/colexec/execpb/stats.proto, line 29 at r1 (raw file):
google.protobuf.Duration time = 4 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; reserved 5;
Why not just rename 5? The semantics are the same and it would avoid having to bump distsql version.
yuzefovich
force-pushed
the
cleanup-stats
branch
from
b7bbd91 to
23b1a4e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/execpb/stats.go, line 54 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Are you sure it includes execution time? I'm under the impression it's only IO.
It's only IO in the row engine, in the vectorized we measure the time differently, so it'll include both IO (cFetcher waiting for next KV) and execution (cFetcher decoding things).
pkg/sql/colexec/execpb/stats.proto, line 29 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Why not just rename 5? The semantics are the same and it would avoid having to bump distsql version.
Done. I did that initially as well but then decided to change to a different value just to be safe, but I've now remembered that I tried renaming a field and it worked like a charm in mixed-version cluster.
There was a problem hiding this comment.
Reviewed 3 of 10 files at r1, 4 of 4 files at r2.
Reviewable status:
pkg/sql/colexec/execpb/stats.go, line 54 at r1 (raw file):
It's only IO in the row engine
I don't think that's true (if you consider "decoding" = "execution time"). rowFetcherStatCollector starts timing before the call to NextRow and stops the timer after. I vote to keep it as just IO, I think it's less confusing because we can't define "execution" time when stalling.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/execpb/stats.go, line 54 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
It's only IO in the row engine
I don't think that's true (if you consider "decoding" = "execution time").
rowFetcherStatCollectorstarts timing before the call toNextRowand stops the timer after. I vote to keep it as justIO, I think it's less confusing because we can't define "execution" time when stalling.
cc @awoods187 @jordanlewis on naming
There was some discussion here https://cockroachlabs.slack.com/archives/C8HD41C82/p1594249854088100?thread_ts=1594243319.067000&cid=C8HD41C82.
Maybe simply "IO time" is less confusing than "IO + execution time", but the latter is more precise IMO.
|
Hmm, I guess I'm happy with whatever you all agreed on. I prefer "IO time" if we can't separate execution in terms of decoding rows, but I might be too close to the code to see the bigger picture. |
There was a problem hiding this comment.
Reviewable status:
|
I don't think separating IO from execution in |
Release note (sql change): `selectivity` information has been removed from EXPLAIN ANALYZE diagrams which would be present if the query was executed via the vectorized engine because it has been quite confusing and probably not helpful information. Release note (sql change): `stall time` has been renamed to `IO time` in EXPLAIN ANALYZE diagrams for the queries that are executed via the vectorized engine.
yuzefovich
force-pushed
the
cleanup-stats
branch
from
23b1a4e to
e7065f7
Compare
|
After discussing with Jordan, we decided to go with Alfonso's suggestion and just to use simply "IO time" since it's less confusing. I'll wait for a green CI and then will merge this. |
|
TFTR! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Release note (sql change):
selectivityinformation has been removedfrom EXPLAIN ANALYZE diagrams which would be present if the query was
executed via the vectorized engine because it has been quite confusing
and probably not helpful information.
Release note (sql change):
stall timehas been renamed toIO + execution timein EXPLAIN ANALYZE diagrams for the queries that areexecuted via the vectorized engine.