distsql: TableReader learns about mutation columns#28551
distsql: TableReader learns about mutation columns#28551jordanlewis wants to merge 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
Previously, TableReaders could never access mutation columns - columns that aren't yet marked public as part of the schema change process. This is problematic for a future where UPDATE gets its input from a TableReader and not a scanNode - the TableReader must be taught about non-public columns. This commit adds a new boolean field to the TableReaderSpec that should be set to true to return non-public columns. When set to true, the TableReader returns all columns, including the mutations. Release note: None
d732983 to
08d8dd5
Compare
solongordon
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/distsql_physical_planner.go, line 1108 at r1 (raw file):
} } if n.desc.Columns[c].ID == n.cols[i].ID {
Is there the danger of an index out of bounds error here if c >= nCols and we didn't break above?
rjnn
left a comment
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1.
Reviewable status:
pkg/sql/distsql_physical_planner.go, line 1080 at r1 (raw file):
for i := range n.desc.Mutations { col := n.desc.Mutations[i].GetColumn() if col != nil {
why do we need this nil check here? in what cases would a nil be returned?
pkg/sql/distsqlrun/processors.proto, line 187 at r1 (raw file):
enum ScanVisibility { PUBLIC = 0; PUBLIC_AND_NOT_PUBLIC = 1;
How about 'MIXED' instead of 'foo AND NOT foo' ?
pkg/sql/distsqlrun/tablereader.go, line 81 at r1 (raw file):
for i := range spec.Table.Mutations { col := spec.Table.Mutations[i].GetColumn() if col != nil {
why do we have to do a nil check here?
jordanlewis
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsql_physical_planner.go, line 1080 at r1 (raw file):
Previously, arjunravinarayan (Arjun Narayan) wrote…
why do we need this
nilcheck here? in what cases would a nil be returned?
When the mutation is an index mutation and not a column mutation
pkg/sql/distsql_physical_planner.go, line 1108 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Is there the danger of an index out of bounds error here if
c >= nColsand we didn't break above?
Yes, this is in fact currently broken. I also cannot figure out how to make it correct. I think I will want to bundle this patch with the partial plan cache PR anyway because it is untested/untestable without that PR.
pkg/sql/distsqlrun/processors.proto, line 187 at r1 (raw file):
Previously, arjunravinarayan (Arjun Narayan) wrote…
How about 'MIXED' instead of 'foo AND NOT foo' ?
I'm copying the language that was used in the non-proto version of this enum.
pkg/sql/distsqlrun/tablereader.go, line 81 at r1 (raw file):
When the mutation is an index mutation and not a column mutation
Release note: None
Previously, TableReaders could never access mutation columns - columns
that aren't yet marked public as part of the schema change process. This
is problematic for a future where UPDATE gets its input from a
TableReader and not a scanNode - the TableReader must be taught about
non-public columns.
This commit adds a new boolean field to the TableReaderSpec that should
be set to true to return non-public columns. When set to true, the
TableReader returns all columns, including the mutations.
Release note: None