distsqlrun: clean up the materializer #39386
Conversation
|
This change is |
|
Let's see what the build says, but I have high hopes that now that |
There was a problem hiding this comment.
Great stuff, this makes sense. Anything run with Flow.Run won't have used this context.
Reviewed 8 of 9 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @yuzefovich)
pkg/sql/distsqlrun/flow.go, line 460 at r1 (raw file):
f.spec = spec if f.isVectorized { log.VEventf(ctx, 1, "setting up vectorize flow %s", f.id.String())
I find it's nicer to log f.id.Short()
pkg/sql/distsqlrun/flow.go, line 485 at r1 (raw file):
// goroutine. The caller must forward any returned error to syncFlowConsumer if // set. func (f *Flow) startInternal(ctx context.Context, doneFn func()) (context.Context, error) {
Add a comment about the context.
pkg/sql/exec/colrpc/outbox.go, line 258 at r1 (raw file):
return nil } log.VEvent(ctx, 2, "Outbox is sending metadata")
I think this message is redundant with respect to what is printed out in movedToDraining. Also, in general we should avoid making unrelated changes in the same commit.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/flow.go, line 460 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I find it's nicer to log
f.id.Short()
I agree, done.
pkg/sql/distsqlrun/flow.go, line 485 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Add a comment about the context.
Done.
pkg/sql/exec/colrpc/outbox.go, line 258 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I think this message is redundant with respect to what is printed out in
movedToDraining. Also, in general we should avoid making unrelated changes in the same commit.
I agree that it's redundant, removed (I forgot to clean this up after debugging).
yuzefovich
force-pushed
the
materializer-cleanup
branch
from
5dec9d1 to
e37e3d2
Compare
|
TFTR! bors r+ |
Merge conflict (retrying...) |
|
bors r- |
Canceled |
yuzefovich
force-pushed
the
materializer-cleanup
branch
from
e37e3d2 to
69e73c3
Compare
This commit fixes a bug of not using the context that was updated during `Flow.startInternal` to run the last processor in the flow. This removes the necessity of having an additional `ctxCancel` function in the materializer. Additionally, `outputToInputColIdx` has been removed from the materializer since it's always a mapping such that o[i] = i. I believe it is a remnant of early days of the vectorized engine, and since then we've been using projection operators to serve the purpose that was initially envisioned for this mapping. Release note: None
yuzefovich
force-pushed
the
materializer-cleanup
branch
from
69e73c3 to
a73eb98
Compare
|
bors r+ |
39159: testutils/lint: convert some checkers to go/analysis r=mjibson a=mjibson Convert float, unconvert, and timer to go/analysis (https://godoc.org/golang.org/x/tools/go/analysis). This is the new, official static analysis framework for Go. These were previously disabled. Although now they work again, they are not yet hooked up. It appears difficult to run these from inside a test. The Go people apparently are steering us toward using the CLI instead. Hence, roachlint. Add roachlint, a program to run these checkers. Not yet hooked up to TestLint. See #33669. Although that mentions using honnef.co/go/tools, that tool now uses go/analysis: https://staticcheck.io/changes/2019.2#go-analysis. Release note: None 39386: distsqlrun: clean up the materializer r=yuzefovich a=yuzefovich This commit fixes a bug of not using the context that was updated during `Flow.startInternal` to run the last processor in the flow. This removes the necessity of having an additional `ctxCancel` function in the materializer. Additionally, `outputToInputColIdx` has been removed from the materializer since it's always a mapping such that o[i] = i. I believe it is a remnant of early days of the vectorized engine, and since then we've been using projection operators to serve the purpose that was initially envisioned for this mapping. Fixes: #39384. Release note: None Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
Build succeeded |
This commit fixes a bug of not using the context that was updated
during
Flow.startInternalto run the last processor in the flow.This removes the necessity of having an additional
ctxCancelfunction in the materializer.
Additionally,
outputToInputColIdxhas been removed from thematerializer since it's always a mapping such that o[i] = i.
I believe it is a remnant of early days of the vectorized engine,
and since then we've been using projection operators to serve
the purpose that was initially envisioned for this mapping.
Fixes: #39384.
Release note: None