colexec: remove a couple of now-stale TODOs #89628
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
-- commits line 7 at r1:
[nit] could this be a PR number instead?
pkg/sql/colexec/colbuilder/execplan.go line 74 at r1 (raw file):
newToWrap func([]execinfra.RowSource) (execinfra.RowSource, error), factory coldata.ColumnFactory, releasables *[]execreleasable.Releasable,
[super nit] I think I prefer passing the slice by value, appending to it and then returning it. Up to you though.
This commit removes a stale TODO about investigating why the materializers could not be released in all cases when they are created to wrap a row-by-row processor into the vectorized flow. The root cause was addressed in cockroachdb#88973 (the problem was that we could call `ConsumerClosed` on an already `Release`d object), so it is now safe to always release the materializers. For the same reason we no longer need to perform a deep copy of the closers when creating the materializer. Additionally, this commit removes a temporary allocation for a slice of releasables by directly modifying the main "tracking" slice. Release note: None
yuzefovich
force-pushed
the
execplan-materializer
branch
from
9f9133d to
d4d5d99
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
Previously, DrewKimball (Drew Kimball) wrote…
[nit] could this be a PR number instead?
Done.
pkg/sql/colexec/colbuilder/execplan.go line 74 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[super nit] I think I prefer passing the slice by value, appending to it and then returning it. Up to you though.
There is already precedent for doing this way in this file, so I'll keep the current way, but please feel to open up a PR to address all places - I don't have strong preference either way.
|
Build succeeded: |
This commit removes a stale TODO about investigating why the
materializers could not be released in all cases when they are created
to wrap a row-by-row processor into the vectorized flow. The root cause
was addressed in #88973 (the problem was that we could call
ConsumerClosedon an alreadyReleased object), so it is now safe toalways release the materializers. For the same reason we no longer need
to perform a deep copy of the closers when creating the materializer.
Additionally, this commit removes a temporary allocation for a slice of
releasables by directly modifying the main "tracking" slice.
Release note: None
Epic: None