opt: add OptionalColList type #58110
Conversation
Release note: None
|
This change is |
There was a problem hiding this comment.
Will you be adding ToSet in a future PR? Is there a use case for it right now?
Reviewed 6 of 6 files at r1, 9 of 9 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, and @RaduBerinde)
pkg/sql/opt/column_meta.go, line 101 at r2 (raw file):
func (cl OptionalColList) Find(col ColumnID) (idx int, ok bool) { for i := range cl { if col != 0 && cl[i] == col {
[nit] you could move a col == 0 check to the top of the function since you'll always return false in that case.
For mutations, we use ColLists that map 1-to-1 to table columns, and where some of the entries in the list can be 0. This is really meant to represent a mapping of columns and it is an abuse of the ColList type (which is supposed to be a simple list of columns). We add a separate OptionalColList type which has the desired semantics. This helps clarify things a bit (in particular it is now obvious which of the lists are "real" lists and which are "mappings"). It will also allow a different ToSet() method which makes sense for this structure (i.e. doesn't put 0 in the set). Release note: None
RaduBerinde
force-pushed
the
col-list-stuff
branch
from
30a2aab to
e447ee9
Compare
There was a problem hiding this comment.
TFTR! I have an upcoming usecase for ToSet, I will add it then.
Reviewable status:
pkg/sql/opt/column_meta.go, line 101 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] you could move a
col == 0check to the top of the function since you'll always returnfalsein that case.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
|
bors r+ |
|
Build failed: |
There was a problem hiding this comment.
Nice!
Reviewable status:
|
bors r+ |
58110: opt: add OptionalColList type r=RaduBerinde a=RaduBerinde I am open to better suggestions for the name. #### opt: change ColSetToList() func to a ToList() method Release note: None #### opt: add OptionalColList type For mutations, we use ColLists that map 1-to-1 to table columns, and where some of the entries in the list can be 0. This is really meant to represent a mapping of columns and it is an abuse of the ColList type (which is supposed to be a simple list of columns). We add a separate OptionalColList type which has the desired semantics. This helps clarify things a bit (in particular it is now obvious which of the lists are "real" lists and which are "mappings"). It will also allow a different ToSet() method which makes sense for this structure (i.e. doesn't put 0 in the set). Release note: None 58156: logictest: fix upsert test r=RaduBerinde a=yuzefovich The trace message appears to contain only a single line if an error occurs (all lines apart from the first one are not shown), so this commit adjusts `upsert` opt logic test accordingly. Fixes: #58158. Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
|
Build failed (retrying...): |
|
Build succeeded: |
I am open to better suggestions for the name.
opt: change ColSetToList() func to a ToList() method
Release note: None
opt: add OptionalColList type
For mutations, we use ColLists that map 1-to-1 to table columns, and
where some of the entries in the list can be 0. This is really meant
to represent a mapping of columns and it is an abuse of the ColList
type (which is supposed to be a simple list of columns).
We add a separate OptionalColList type which has the desired
semantics. This helps clarify things a bit (in particular it is now
obvious which of the lists are "real" lists and which are "mappings").
It will also allow a different ToSet() method which makes sense for
this structure (i.e. doesn't put 0 in the set).
Release note: None