exec: fix planning of count operator #38767
Conversation
|
This change is |
There was a problem hiding this comment.
Fix for COUNT
This is underscoring for me that I think it's time to think about refactoring the column exec setup stuff. I think we should move it into a different package and give each case in this giant switch its own function, so that the code doesn't have to "remember" stuff like not being allowed to just return.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @solongordon and @yuzefovich)
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 65 at r1 (raw file):
vec := batch.ColVec(p.colIdx) col := vec.{{.LTyp}}()[:coldata.BatchSize] projVec := batch.ColVec(p.outputIdx)
I think the purpose of these slices was to eliminate bounds checks
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 73 at r1 (raw file):
} else { col := vec.{{.LTyp}}()[:n] projCol := projVec.{{.RetTyp}}()[:n]
I think the purpose of the len(col)-1 thing was to eliminate bounds checks.
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 110 at r1 (raw file):
projVec := batch.ColVec(p.outputIdx) if sel := batch.Selection(); sel != nil { col := vec.{{.RTyp}}()[:coldata.BatchSize]
I'm confused why it's okay to do this slicing up to batch size in the case of sel?
yuzefovich
force-pushed
the
fix-count
branch
2 times, most recently
from
846adb8
to
5375301
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 65 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I think the purpose of these slices was to eliminate bounds checks
Yeah, I knew that. I took a look at compiled code and didn't see a noticeable change. But I was wrong - I should've used check_bce flag and look at the number lines as Solon suggested. Now, I've checked the three places where I'm making modifications, and bounds checks are eliminated in all of them.
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 73 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I think the purpose of the len(col)-1 thing was to eliminate bounds checks.
Done.
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 110 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I'm confused why it's okay to do this slicing up to batch size in the case of sel?
Well, it's not really ok if the underlying batch does not have the required capacity. I guess count operator is the only one (?) that uses a batch with not 1024 capacity, and if we put a filter on top of it and then a projection, it'll fail.
I checked, and top k also uses a custom capacity.
Selections were also prone to this problem. I'm not sure whether slicing up to capacity is the right thing here though. Thoughts?
|
Guys, please take another look. |
There was a problem hiding this comment.
Good catch that we should not be arbitrarily slicing up to BatchSize. However I'm not sure that the new bounds check elimination stuff you added is really doing anything. See my inline comments.
Reviewable status:
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 72 at r2 (raw file):
} else { col = col[:coldata.BatchSize] }
I'm not convinced there's any bounds check being eliminated here. It seems like this would only eliminate a bounds check if the compiler knew that i in the loop below is less than cap(col) and coldata.BatchSize, which it cannot. I tried getting rid of these 5 lines and did not see any more bounds checks when I did check_bce. Same goes for the other selection vector cases.
I don't see a way to do BCE here, so I would just get rid of the slicing entirely.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 65 at r2 (raw file):
vec := batch.ColVec(p.colIdx) projVec := batch.ColVec(p.outputIdx) projCol := projVec.{{.RetTyp}}()[:coldata.BatchSize]
While we're at it I think this [:coldata.BatchSize] slice is also pointless. (Same goes for the other one below.)
There was a problem hiding this comment.
BTW, I think select_in_tmpl.go has the same BatchSize issue if you'd like to fix that here too.
Reviewable status:
Previously, when planning a count operator, we would add it to the flow and would ignore any post-operator planning (like projections). Now, this is fixed. Additionally, this commit fixes slicing within projections operators - previously, we would always slice up to BatchSize, but the underlying memory not always has sufficient capacity (for example, count operator uses a batch with a capacity of 1) which would cause an index out of bounds. Release note: None
There was a problem hiding this comment.
Hm, I didn't even know about select_in_tmpl :)
Reviewable status:
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 65 at r2 (raw file):
Previously, solongordon (Solon) wrote…
While we're at it I think this
[:coldata.BatchSize]slice is also pointless. (Same goes for the other one below.)
Done.
pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 72 at r2 (raw file):
Previously, solongordon (Solon) wrote…
I'm not convinced there's any bounds check being eliminated here. It seems like this would only eliminate a bounds check if the compiler knew that
iin the loop below is less thancap(col)andcoldata.BatchSize, which it cannot. I tried getting rid of these 5 lines and did not see any more bounds checks when I didcheck_bce. Same goes for the other selection vector cases.I don't see a way to do BCE here, so I would just get rid of the slicing entirely.
Makes sense, removed.
There was a problem hiding this comment.
BatchSize stuff goes back to some cargo culting on my part before I understood how BCE works, so thanks for cleaning up my mess. :)
Reviewable status:
|
Thanks for the review! I was glad to take a look at this stuff to learn more about BCE myself :) bors r+ |
38767: exec: fix planning of count operator r=yuzefovich a=yuzefovich Previously, when planning a count operator, we would add it to the flow and would ignore any post-operator planning (like projections). Now, this is fixed. Additionally, this commit fixes slicing within projections operators - previously, we would always slice up to BatchSize, but the underlying memory not always has sufficient capacity (for example, count operator uses a batch with a capacity of 1) which would cause an index out of bounds. Fixes: #38752. Release note: None 38881: exec: fix NaN comparison logic r=solongordon a=solongordon I added special NaN handling for float comparisons. In SQL, NaNs are treated as less than any other float value. Thankfully I'm not seeing a performance hit when I run our sort benchmarks with float64 values. Fixes #38751 Release note: None 38891: c-deps: bump rocksdb for macOS build fix r=ajkr a=ajkr Pick up cockroachdb/rocksdb#39 Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Solon Gordon <solon@cockroachlabs.com> Co-authored-by: Andrew Kryczka <andrew.kryczka2@gmail.com>
Build succeeded |
Previously, when planning a count operator, we would add it to the
flow and would ignore any post-operator planning (like projections).
Now, this is fixed.
Additionally, this commit fixes slicing within projections operators -
previously, we would always slice up to BatchSize, but the underlying
memory not always has sufficient capacity (for example, count operator
uses a batch with a capacity of 1) which would cause an index out of
bounds.
Fixes: #38752.
Release note: None