colbuilder: don't use direct scans in an invalid case #145255
Conversation
|
This change is |
yuzefovich
force-pushed
the
fix-direct-scans
branch
2 times, most recently
from
afa391b to
48830f0
Compare
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/colbuilder/execplan.go line 896 at r1 (raw file):
return false } if l, err := keys.GetRowPrefixLength(core.TableReader.Spans[i].Key); err == nil {
nit: it seems fine to just return false if we fail to get the length of a key - is there a useful case where we expect an error?
yuzefovich
force-pushed
the
fix-direct-scans
branch
from
48830f0 to
d182f77
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/colexec/colbuilder/execplan.go line 896 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
nit: it seems fine to just return false if we fail to get the length of a key - is there a useful case where we expect an error?
Good point, done.
145255: colbuilder: don't use direct scans in an invalid case r=yuzefovich a=yuzefovich We have an experimental feature to build `coldata.Batch`es directly on the KV server side when handling Scan and ReverseScan requests. It assumes that each request corresponds to any number of full SQL rows, but previously we didn't disable usage of this feature when a single SQL row is populated via multiple requests. This is the case when we have multiple column families, and since we don't support Gets in the direct scans machinery right now, we need at least 5 column families - so that we get Scan for 0-1 and 3-4 CFs. This commit fixes this oversight to ensure that if we have two consecutive key spans that come from the same SQL row, we don't use the direct scans even if they are enabled. There is no release note since this feature is disabled by default. Fixes: #145232. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
|
Build failed: |
We have an experimental feature to build `coldata.Batch`es directly on the KV server side when handling Scan and ReverseScan requests. It assumes that each request corresponds to any number of full SQL rows, but previously we didn't disable usage of this feature when a single SQL row is populated via multiple requests. This is the case when we have multiple column families, and since we don't support Gets in the direct scans machinery right now, we need at least 5 column families - so that we get Scan for 0-1 and 3-4 CFs. This commit fixes this oversight to ensure that if we have two consecutive key spans that come from the same SQL row, we don't use the direct scans even if they are enabled. There is no release note since this feature is disabled by default. Release note: None
yuzefovich
force-pushed
the
fix-direct-scans
branch
from
d182f77 to
18a3fc3
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/sql/colexec/colbuilder/execplan.go line 896 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Good point, done.
Turns out that we can return an error if we manually added a split point to the range, which is why I had to adjust one test. It still seems fine to just disable direct scans.
We have an experimental feature to build
coldata.Batches directly on the KV server side when handling Scan and ReverseScan requests. It assumes that each request corresponds to any number of full SQL rows, but previously we didn't disable usage of this feature when a single SQL row is populated via multiple requests. This is the case when we have multiple column families, and since we don't support Gets in the direct scans machinery right now, we need at least 5 column families - so that we get Scan for 0-1 and 3-4 CFs. This commit fixes this oversight to ensure that if we have two consecutive key spans that come from the same SQL row, we don't use the direct scans even if they are enabled.There is no release note since this feature is disabled by default.
Fixes: #145232.
Release note: None