exec: distinct manages its own scratch column#31881
exec: distinct manages its own scratch column#31881craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
cf5d204 to
22b236e
Compare
asubiotto
left a comment
There was a problem hiding this comment.
Mind running the benchmarks? Don't expect anything to change but I think it's good to get into the habit of doing this when modifying operators.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/sql/exec/distinct.go, line 31 at r1 (raw file):
sortedDistinctCol int // outputColIdx is the boolean output column.
s/outputColIdx/outputCol
pkg/sql/exec/distinct.go, line 50 at r1 (raw file):
// input columns with the given types. func NewOrderedDistinct(input Operator, distinctCols []uint32, typs []types.T) (Operator, error) { distinctCol := make([]bool, ColBatchSize)
Maybe add a comment about how distinctCol is shared between many operators?
pkg/sql/exec/distinct.go, line 138 at r1 (raw file):
input Operator // outputColIdx is the boolean output column from previous
s/outputColIdx/outputCol
pkg/sql/exec/distinct_test.go, line 56 at r1 (raw file):
for _, tc := range tcs { runTests(t, tc.tuples, []types.T{}, func(t *testing.T, input Operator) { distinct, err := NewOrderedDistinct(input, tc.distinctCols, tc.colTypes)
Do you need to call Init on this?
jordanlewis
left a comment
There was a problem hiding this comment.
BenchmarkSortedDistinct-8 500000 2834 ns/op 8671.00 MB/s
they're the same.
TFTRs!
Reviewable status:
pkg/sql/exec/distinct.go, line 31 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
s/outputColIdx/outputCol
Done.
pkg/sql/exec/distinct.go, line 50 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Maybe add a comment about how
distinctColis shared between many operators?
Done.
pkg/sql/exec/distinct.go, line 138 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
s/outputColIdx/outputCol
Done.
pkg/sql/exec/distinct_test.go, line 56 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Do you need to call
Initon this?
No, the test framework automatically inits the bottom op in the chain, which will propagate everywhere.
Previously, distinct relied on its input batch to have a scratch boolean column for working. It's unnecessary - instead, manage the scratch boolean directly during construction. Release note: None
22b236e to
758d1c7
Compare
|
bors r+ |
31881: exec: distinct manages its own scratch column r=jordanlewis a=jordanlewis Previously, distinct relied on its input batch to have a scratch boolean column for working. It's unnecessary - instead, manage the scratch boolean directly during construction. Release note: None 31926: storage: adjust raft log size correctly when replacing sideloaded entries r=nvanbenschoten a=tschottdorf This follows up on a comment of @nvanbenschoten in #31914 which highlighted yet another potential (though hopefully rare) source of raft log size not being reduced correctly. Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Build succeeded |
Previously, distinct relied on its input batch to have a scratch boolean
column for working. It's unnecessary - instead, manage the scratch
boolean directly during construction.
Release note: None