rowexec: fix a bug with distinct on arrays and composite types#44771
rowexec: fix a bug with distinct on arrays and composite types#44771rohany wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
As @solongordon pointed out, this actually affected all types where the component that we sort by is not equal to the real value, such as composite types. I'll update the message to reflect this. |
9139d5d to
56d8ed3
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
56d8ed3 to
58be03d
Compare
|
@yuzefovich can you PTAL? I made a subtle change here to fix some tests that I want another pair of eyes on. |
yuzefovich
left a comment
There was a problem hiding this comment.
The change looks good to me, but I'm not that familiar with encodings, I think you should ping someone else (maybe Solon) to take another quick look. I assume that this PR will also need to be backported, so an extra attention wouldn't hurt.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/sqlbase/encoded_datum.go, line 280 at r2 (raw file):
// the existing value encoding may include the column ID within the encoding. This function is intended // to be used when a column ID free value encoding is desired. func (ed *EncDatum) EncodeAsValueWithNoColID(
It'd be nice to unit test this function.
|
cc @solongordon |
58be03d to
5ac4568
Compare
rohany
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlbase/encoded_datum.go, line 280 at r2 (raw file):
Previously, yuzefovich wrote…
It'd be nice to unit test this function.
Added a simple unit test
|
hmm, it seems like this is not the correct fix. Looking at postgres: Switching to the value encoding would not result in this behavior. It seems like this should actually just be related to the key encoding for arrays, and making sure that the key encoding for an array of null is different than the encoding for null. |
5ac4568 to
0596a44
Compare
Fixes cockroachdb#44079. This PR fixes a bug caused by the table encoding of ARRAY types. In particular, NULL had the same encoding as ARRAY[NULL]. Release note (bug fix): Fix a bug where the distinct operation on ARRAY[NULL] and NULL could sometimes return an incorrect result and omit some tuples.
0596a44 to
b482fb2
Compare
|
PTAL, now that the distinct vec bug has been fixed |
yuzefovich
left a comment
There was a problem hiding this comment.
LGTM.
Reviewed 6 of 6 files at r3.
Reviewable status:
|
I'm actually going to close this in favor of another solution. |
Fixes #44079.
This PR fixes a bug caused by the table encoding of
ARRAY types. In particular, NULL had the same encoding
as ARRAY[NULL].
Release note (bug fix): Fix a bug where the distinct operation
on ARRAY[NULL] and NULL could sometimes
return an incorrect result and omit some tuples.