sql: fix decoding error in fetcher#43002
Conversation
|
This change is |
|
@yuzefovich you've done some work in past with random tests / tests using all types -- do you know of something that could help test my change to |
jordanlewis
left a comment
There was a problem hiding this comment.
Can you add tests for SkipTableKey under TestEncodeTableKey? It does a randomized property based test already, you can add the SkipTableKey by asserting that it skips the right number of bytes.
Code LGTM. The commit message looks like it's missing an opening sentence.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
|
Added a test, PTAL! |
yuzefovich
left a comment
There was a problem hiding this comment.
Thanks for the quick fix!
Reviewed 6 of 6 files at r1.
Reviewable status:
Fixes cockroachdb#42992. PR cockroachdb#42073 introduced column families for secondary indexes, but also introduced a bug where if the extra columns of a unique index were included because that row had a null value, then those extra columns needed to be skipped when decoding keys. If the type of that extra column was not supported by the vectorized engine, and internal error was thrown. This PR fixes the bug by updating the key skipping logic to handle all valid types. There is no release note because this was not a change visible through major versions. Release note: None
|
This affects any unsupported types that are part of a key, right? This probably needs to be backported with #42934 |
|
OK, should I backport #42934 as it is and then let you backport that change separately? |
|
bors r=jordanlewis |
43002: sql: fix decoding error in fetcher r=jordanlewis a=rohany Fixes #42992. PR #42073 introduced column families for secondary indexes, but also introduced a bug where if the extra columns of a unique index were included because that row had a null value, then those extra columns needed to be skipped when decoding keys. If the type of that extra column was not supported by the vectorized engine, and internal error was thrown. This PR fixes the bug by updating the key skipping logic to handle all valid types. There is no release note because this was not a change visible through major versions. Release note: None Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Build succeeded |
Fixes #42992.
PR #42073 introduced column families for secondary indexes, but also introduced
a bug where if the extra columns of a unique index were included because that
row had a null value, then those extra columns needed to be skipped when
decoding keys. If the type of that extra column was not supported by the
vectorized engine, and internal error was thrown. This PR fixes the bug by
updating the key skipping logic to handle all valid types.
There is no release note because this was not a change visible
through major versions.
Release note: None