-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
coldata: bugfix to coldata.Copy with NULL and SelOnDest #40620
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/col/coldata/vec_test.go, line 366 at r1 (raw file):
// Set up the destination vector. dst := NewMemColumn(typ, BatchSize)
[nit]: I'd mention that the vector is with all nulls except for a single non-null at the index 0.
pkg/col/coldata/vec_test.go, line 375 at r1 (raw file):
// Set up the source vector with one null. src := NewMemColumn(typ, BatchSize)
[nit]: s/one null/two nulls/
above.
pkg/col/coldata/vec_test.go, line 391 at r1 (raw file):
Src: src, SrcStartIdx: 1, SrcEndIdx: 2,
Comment below indicates that you might have intended to use SrcEndIdx: 3
.
pkg/col/coldata/vec_tmpl.go, line 131 at r1 (raw file):
// } switch args.ColType {
[nit]: s/indicate/indicates/
above.
9195569
to
ae0885d
Compare
Previously, Copy always wiped all of the destination NULLs regardless of SelOnDest, which if set indiciates that only the selection vector tuples will be written to on the destinatiion side. Now, when SelOnDest is true, we don't erroneously overwrite non-selected nulls. Release note: None
ae0885d
to
99711d9
Compare
bors r+ |
Canceled (will resume) |
Build failed |
bors r+ |
40620: coldata: bugfix to coldata.Copy with NULL and SelOnDest r=jordanlewis a=jordanlewis Previously, Copy always wiped all of the destination NULLs regardless of SelOnDest, which if set indiciates that only the selection vector tuples will be written to on the destination side. Now, when SelOnDest is true, we don't erroneously overwrite non-selected nulls. Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Build succeeded |
Previously, Copy always wiped all of the destination NULLs regardless of
SelOnDest, which if set indiciates that only the selection vector tuples
will be written to on the destination side. Now, when SelOnDest is
true, we don't erroneously overwrite non-selected nulls.
Release note: None