exec: explicitly check for nulls in selBoolOp #40610
Conversation
|
This change is |
There was a problem hiding this comment.
lgtm, just had one potential optional optimization
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @yuzefovich)
pkg/sql/exec/bool_vec_to_sel.go, line 147 at r1 (raw file):
} } else { for i := uint16(0); i < n; i++ {
not sure about this, but is there a bounds-check elimination opportunity here? like if the loop were changed to something like:
outSlice := d.boolVecToSelOp.outputCol[:n]
for i := range outSlice {
if nulls.NullAt(i) {
outSlice[i] = false
}
}
Previously, we would copy the bool vector from boolVecToSelOp directly. However, we also need to look at the null values so that nulls are treated as false. Now this is fixed. Release note: None
yuzefovich
force-pushed
the
fix-selboolop
branch
from
257f441 to
7a382a2
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/exec/bool_vec_to_sel.go, line 147 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
not sure about this, but is there a bounds-check elimination opportunity here? like if the loop were changed to something like:
outSlice := d.boolVecToSelOp.outputCol[:n] for i := range outSlice { if nulls.NullAt(i) { outSlice[i] = false } }
Done. I initially didn't want to look into this, so I left a TODO :) But I guess it's a quick check to see whether the bounds check is eliminated, so your nudge changed my mind.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
Build failed |
|
The logic tests seem to have timed out. bors r+ |
40610: exec: explicitly check for nulls in selBoolOp r=yuzefovich a=yuzefovich Previously, we would copy the bool vector from boolVecToSelOp directly. However, we also need to look at the null values so that nulls are treated as false. Now this is fixed. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
Build succeeded |
Previously, we would copy the bool vector from boolVecToSelOp directly.
However, we also need to look at the null values so that nulls are
treated as false. Now this is fixed.
Release note: None