diff --git a/pkg/sql/colexec/colbuilder/execplan_test.go b/pkg/sql/colexec/colbuilder/execplan_test.go index d59ce72fb0c5..d2623c15f78c 100644 --- a/pkg/sql/colexec/colbuilder/execplan_test.go +++ b/pkg/sql/colexec/colbuilder/execplan_test.go @@ -84,9 +84,8 @@ func TestNewColOperatorExpectedTypeSchema(t *testing.T) { desc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "t") tr := execinfrapb.TableReaderSpec{ - Table: *desc.TableDesc(), - Spans: make([]roachpb.Span, 1), - NeededColumns: []uint32{0}, + Table: *desc.TableDesc(), + Spans: make([]roachpb.Span, 1), } var err error tr.Spans[0].Key, err = randgen.TestingMakePrimaryIndexKey(desc, 0) diff --git a/pkg/sql/colexec/colexecargs/BUILD.bazel b/pkg/sql/colexec/colexecargs/BUILD.bazel index 1c16d03209c5..34047e3c0b59 100644 --- a/pkg/sql/colexec/colexecargs/BUILD.bazel +++ b/pkg/sql/colexec/colexecargs/BUILD.bazel @@ -16,7 +16,6 @@ go_library( "//pkg/sql/colexecop", "//pkg/sql/execinfra", "//pkg/sql/execinfrapb", - "//pkg/sql/parser", "//pkg/sql/sem/tree", "//pkg/sql/types", "//pkg/util/mon", diff --git a/pkg/sql/colexec/colexecargs/expr.go b/pkg/sql/colexec/colexecargs/expr.go index b14d0396de27..f524715e045e 100644 --- a/pkg/sql/colexec/colexecargs/expr.go +++ b/pkg/sql/colexec/colexecargs/expr.go @@ -14,7 +14,6 @@ import ( "sync" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" - "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -51,55 +50,3 @@ func (h *ExprHelper) ProcessExpr( tempVars := tree.MakeIndexedVarHelper(&h.helper, len(typs)) return execinfrapb.DeserializeExpr(expr.Expr, h.SemaCtx, evalCtx, &tempVars) } - -// Remove unused warning. -var _ = findIVarsInRange - -// findIVarsInRange searches Expr for presence of tree.IndexedVars with indices -// in range [start, end). It returns a slice containing all such indices. -func findIVarsInRange(expr execinfrapb.Expression, start int, end int) ([]uint32, error) { - res := make([]uint32, 0) - if start >= end { - return res, nil - } - var exprToWalk tree.Expr - if expr.LocalExpr != nil { - exprToWalk = expr.LocalExpr - } else { - e, err := parser.ParseExpr(expr.Expr) - if err != nil { - return nil, err - } - exprToWalk = e - } - visitor := ivarExpressionVisitor{ivarSeen: make([]bool, end)} - _, _ = tree.WalkExpr(visitor, exprToWalk) - for i := start; i < end; i++ { - if visitor.ivarSeen[i] { - res = append(res, uint32(i)) - } - } - return res, nil -} - -type ivarExpressionVisitor struct { - ivarSeen []bool -} - -var _ tree.Visitor = &ivarExpressionVisitor{} - -// VisitPre is a part of tree.Visitor interface. -func (i ivarExpressionVisitor) VisitPre(expr tree.Expr) (bool, tree.Expr) { - switch e := expr.(type) { - case *tree.IndexedVar: - if e.Idx < len(i.ivarSeen) { - i.ivarSeen[e.Idx] = true - } - return false, expr - default: - return true, expr - } -} - -// VisitPost is a part of tree.Visitor interface. -func (i ivarExpressionVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr } diff --git a/pkg/sql/colfetcher/BUILD.bazel b/pkg/sql/colfetcher/BUILD.bazel index a4cb289bbc1c..3ab245fefc10 100644 --- a/pkg/sql/colfetcher/BUILD.bazel +++ b/pkg/sql/colfetcher/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//pkg/sql/colencoding", "//pkg/sql/colexec/colexecargs", "//pkg/sql/colexec/colexecspan", + "//pkg/sql/colexec/colexecutils", "//pkg/sql/colexecerror", "//pkg/sql/colexecop", "//pkg/sql/colmem", diff --git a/pkg/sql/colfetcher/cfetcher_setup.go b/pkg/sql/colfetcher/cfetcher_setup.go index 54be42f9428e..d1e59cea6ca8 100644 --- a/pkg/sql/colfetcher/cfetcher_setup.go +++ b/pkg/sql/colfetcher/cfetcher_setup.go @@ -16,9 +16,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecargs" + "github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecutils" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/physicalplan" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util" ) @@ -71,26 +73,12 @@ func (a *cFetcherTableArgs) populateTypes(cols []catalog.Column) { } } -// populateTableArgs fills all fields of the cFetcherTableArgs except for -// ColIdxMap. Note that all columns accessible from the index (i.e. present in -// the key or value part) will be included in the result. In order to prune -// the unnecessary columns away, use keepOnlyNeededColumns. -// -// If index is a secondary index, then all inaccessible columns are pruned away. -// In such a scenario a non-nil idxMap is returned that allows to remap ordinals -// referring to columns from the whole table to the correct positions among only -// accessible columns. post will be adjusted automatically. Columns that are -// not accessible from the secondary index have an undefined value corresponding -// to them if idxMap is non-nil. -// -// For example, say the table has 4 columns (@1, @2, @3, @4), but only 2 columns -// are present in the index we're reading from (@3, @1). In this case, the -// returned table args only contains columns (@1, @3) and we get an index map as -// idxMap = [0, x, 1, x] (where 'x' indicates an undefined value). -// Note that although @3 appears earlier than @1 in the index, because we -// iterate over all columns of the table according to their column ordinals, we -// will see @1 first, so it gets the 0th slot, and @3 second, so it gets the 1st -// slot. +// populateTableArgs fills all fields of the cFetcherTableArgs. It examines the +// given post-processing spec to find the set of the needed columns, and only +// these columns are added into the table args while post is adjusted +// accordingly. +// - neededColumns is a set containing the ordinals of all columns that need to +// be fetched. func populateTableArgs( ctx context.Context, flowCtx *execinfra.FlowCtx, @@ -101,7 +89,7 @@ func populateTableArgs( hasSystemColumns bool, post *execinfrapb.PostProcessSpec, helper *colexecargs.ExprHelper, -) (_ *cFetcherTableArgs, idxMap []int, _ error) { +) (_ *cFetcherTableArgs, neededColumns util.FastIntSet, _ error) { args := cFetcherTableArgsPool.Get().(*cFetcherTableArgs) // First, find all columns present in the table and possibly include the // system columns (when requested). @@ -119,44 +107,44 @@ func populateTableArgs( } } } - numSystemCols := 0 if hasSystemColumns { - systemCols := table.SystemColumns() - numSystemCols = len(systemCols) - cols = append(cols, systemCols...) + cols = append(cols, table.SystemColumns()...) } - if !index.Primary() { - // If we have a secondary index, not all columns might be available from - // the index, so we'll prune the unavailable columns away. - colIDs := index.CollectKeyColumnIDs() - colIDs.UnionWith(index.CollectSecondaryStoredColumnIDs()) - colIDs.UnionWith(index.CollectKeySuffixColumnIDs()) - if colIDs.Len() < len(cols)-numSystemCols { - needTypesBeforeRemapping := post.RenderExprs != nil - if needTypesBeforeRemapping { - args.populateTypes(cols) - } - idxMap = make([]int, len(cols)) - colIdx := 0 - for i := range cols { - //gcassert:bce - id := cols[i].GetID() - if colIDs.Contains(id) || (hasSystemColumns && i >= len(cols)-numSystemCols) { - idxMap[i] = colIdx - cols[colIdx] = cols[i] - colIdx++ - } - } - cols = cols[:colIdx] - if err := remapPostProcessSpec( - flowCtx, post, idxMap, helper, args.typs, - ); err != nil { - return nil, nil, err + var err error + // Make sure that render expressions are deserialized right away so that we + // don't have to re-parse them multiple times. + if post.RenderExprs != nil { + args.populateTypes(cols) + for i := range post.RenderExprs { + // It is ok to use the evalCtx of the flowCtx since it won't be + // mutated (we are not evaluating the expressions). It's also ok to + // update post in-place even if flowCtx.PreserveFlowSpecs is true + // since we're not really mutating the render expressions. + post.RenderExprs[i].LocalExpr, err = helper.ProcessExpr(post.RenderExprs[i], flowCtx.EvalCtx, args.typs) + if err != nil { + return args, neededColumns, err } } } + // Now find the set of columns that are actually needed based on the + // post-processing spec. + neededColumns = getNeededColumns(post, len(cols)) + + // Prune away columns that aren't needed. + if neededColumns.Len() != len(cols) { + idxMap := make([]int, len(cols)) + keepColIdx := 0 + for idx, ok := neededColumns.Next(0); ok; idx, ok = neededColumns.Next(idx + 1) { + cols[keepColIdx] = cols[idx] + idxMap[idx] = keepColIdx + keepColIdx++ + } + cols = cols[:keepColIdx] + remapPostProcessSpec(post, idxMap, flowCtx.PreserveFlowSpecs) + } + *args = cFetcherTableArgs{ desc: table, index: index, @@ -165,114 +153,59 @@ func populateTableArgs( typs: args.typs, } args.populateTypes(cols) + for i := range cols { + args.ColIdxMap.Set(cols[i].GetID(), i) + } // Before we can safely use types from the table descriptor, we need to // make sure they are hydrated. In row execution engine it is done during // the processor initialization, but neither ColBatchScan nor cFetcher are // processors, so we need to do the hydration ourselves. resolver := flowCtx.TypeResolverFactory.NewTypeResolver(flowCtx.Txn) - return args, idxMap, resolver.HydrateTypeSlice(ctx, args.typs) + return args, neededColumns, resolver.HydrateTypeSlice(ctx, args.typs) } -// keepOnlyNeededColumns updates the tableArgs to prune all unnecessary columns -// away based on neededColumns slice. If we're reading of the secondary index -// that is not covering all columns, idxMap must be non-nil describing the -// remapping that needs to be used for column ordinals from neededColumns. -// post is updated accordingly to refer to new ordinals of columns. The method -// also populates tableArgs.ColIdxMap. -func keepOnlyNeededColumns( - flowCtx *execinfra.FlowCtx, - tableArgs *cFetcherTableArgs, - idxMap []int, - neededColumns []uint32, - post *execinfrapb.PostProcessSpec, - helper *colexecargs.ExprHelper, -) error { - if len(neededColumns) < len(tableArgs.cols) { - // If we don't need all of the available columns, we will prune all of - // the not needed columns away. - - // First, populate a set of needed columns. - var neededColumnsSet util.FastIntSet - for _, neededColumn := range neededColumns { - neededColIdx := int(neededColumn) - if idxMap != nil { - neededColIdx = idxMap[neededColIdx] - } - neededColumnsSet.Add(neededColIdx) - } - - // When idxMap is non-nil, we can reuse that. Note that in this case - // the length of idxMap is equal to the number of columns in the - // whole table, and we are reading from the secondary index, so the - // slice will have the sufficient size. We also don't need to reset - // it since we'll update the needed positions below. - if idxMap == nil { - idxMap = make([]int, len(tableArgs.typs)) - } - - // Iterate over all needed columns, populate the idxMap, and adjust - // the post-processing spec to refer only to the needed columns - // directly. - // - // If non-nil idxMap was passed into this method, we have to update it - // by essentially applying a projection on top of the already present - // projection. Consider the following example: - // idxMap = [0, x, 1, x] (where 'x' indicates an undefined value) - // and - // neededColumns = [2]. - // Such a setup means that only columns with ordinals @1 and @3 are - // present in the secondary index while only @3 is actually needed. - // Above, we have already remapped neededColIdx = 2 to be 1, so now - // neededColumnsSet only contains 1. The post-processing already refers - // to this column as having index 1. - // However, since we are pruning the column with index 0 away, the - // post-processing stage will see a single column. Thus, we have to - // update the index map to be - // idxMap = [x, 0, x, x] - // and then remap the post-processing spec below so that it refers to - // the single needed column with the correct ordinal. - neededColIdx := 0 - for idx, ok := neededColumnsSet.Next(0); ok; idx, ok = neededColumnsSet.Next(idx + 1) { - idxMap[idx] = neededColIdx - neededColIdx++ - } - if err := remapPostProcessSpec( - flowCtx, post, idxMap, helper, tableArgs.typs, - ); err != nil { - return err +// getNeededColumns returns the set of needed columns that a processor core must +// output because these columns are used by the post-processing stage. It is +// assumed that the render expressions, if any, have already been deserialized. +func getNeededColumns(post *execinfrapb.PostProcessSpec, numColumns int) util.FastIntSet { + var neededColumns util.FastIntSet + if !post.Projection && len(post.RenderExprs) == 0 { + // All columns are needed. + neededColumns.AddRange(0, numColumns-1) + } else if post.Projection { + for _, neededColOrd := range post.OutputColumns { + neededColumns.Add(int(neededColOrd)) } - - // Now we have to actually prune out the unnecessary columns. - neededColIdx = 0 - for idx, ok := neededColumnsSet.Next(0); ok; idx, ok = neededColumnsSet.Next(idx + 1) { - tableArgs.cols[neededColIdx] = tableArgs.cols[idx] - tableArgs.typs[neededColIdx] = tableArgs.typs[idx] - neededColIdx++ + } else { + var visitor ivarExpressionVisitor + for _, expr := range post.RenderExprs { + visitor.ivarSeen = colexecutils.MaybeAllocateBoolArray(visitor.ivarSeen, numColumns) + _, _ = tree.WalkExpr(visitor, expr.LocalExpr) + for i, seen := range visitor.ivarSeen { + if seen { + neededColumns.Add(i) + } + } + if neededColumns.Len() == numColumns { + // All columns are needed, so we can stop processing the + // subsequent render expressions. + break + } } - tableArgs.cols = tableArgs.cols[:neededColIdx] - tableArgs.typs = tableArgs.typs[:neededColIdx] } - - // Populate the ColIdxMap. - for i := range tableArgs.cols { - tableArgs.ColIdxMap.Set(tableArgs.cols[i].GetID(), i) - } - return nil + return neededColumns } // remapPostProcessSpec updates post so that all IndexedVars refer to the new -// ordinals according to idxMap. +// ordinals according to idxMap. It is assumed that the render expressions, if +// any, have already been deserialized. // // For example, say we have idxMap = [0, 0, 1, 2, 0, 0] and a render expression // like '(@1 + @4) / @3`, then it'll be updated into '(@1 + @3) / @2'. Such an // idxMap indicates that the table has 6 columns and only 3 of them (0th, 2nd, // 3rd) are needed. // -// typsBeforeRemapping need to contain all the types of columns before the -// mapping of idxMap was applied. These will only be used if post.RenderExprs is -// not nil. -// // If preserveFlowSpecs is true, then this method updates post to store the // original output columns or render expressions. Notably, in order to not // corrupt the flow specs that have been scheduled to run on the remote nodes, @@ -283,16 +216,10 @@ func keepOnlyNeededColumns( // which occurs **after** we have sent out SetupFlowRequest RPCs. In other // words, every node must have gotten the unmodified version of the spec and is // now free to modify it as it pleases. -func remapPostProcessSpec( - flowCtx *execinfra.FlowCtx, - post *execinfrapb.PostProcessSpec, - idxMap []int, - helper *colexecargs.ExprHelper, - typsBeforeRemapping []*types.T, -) error { +func remapPostProcessSpec(post *execinfrapb.PostProcessSpec, idxMap []int, preserveFlowSpecs bool) { if post.Projection { outputColumns := post.OutputColumns - if flowCtx.PreserveFlowSpecs && post.OriginalOutputColumns == nil { + if preserveFlowSpecs && post.OriginalOutputColumns == nil { // This is the first time we're modifying this PostProcessSpec, but // we've been asked to preserve the specs, so we have to set the // original output columns. We are also careful to allocate a new @@ -305,7 +232,7 @@ func remapPostProcessSpec( } } else if post.RenderExprs != nil { renderExprs := post.RenderExprs - if flowCtx.PreserveFlowSpecs && post.OriginalRenderExprs == nil { + if preserveFlowSpecs && post.OriginalRenderExprs == nil { // This is the first time we're modifying this PostProcessSpec, but // we've been asked to preserve the specs, so we have to set the // original render expressions. We are also careful to allocate a @@ -313,19 +240,30 @@ func remapPostProcessSpec( post.OriginalRenderExprs = renderExprs post.RenderExprs = make([]execinfrapb.Expression, len(renderExprs)) } - var err error for i := range renderExprs { - // Make sure that the render expression is deserialized if we - // are on the remote node. - // - // It is ok to use the evalCtx of the flowCtx since it won't be - // mutated (we are not evaluating the expressions). - post.RenderExprs[i].LocalExpr, err = helper.ProcessExpr(renderExprs[i], flowCtx.EvalCtx, typsBeforeRemapping) - if err != nil { - return err - } post.RenderExprs[i].LocalExpr = physicalplan.RemapIVarsInTypedExpr(renderExprs[i].LocalExpr, idxMap) } } - return nil } + +type ivarExpressionVisitor struct { + ivarSeen []bool +} + +var _ tree.Visitor = &ivarExpressionVisitor{} + +// VisitPre is a part of tree.Visitor interface. +func (i ivarExpressionVisitor) VisitPre(expr tree.Expr) (bool, tree.Expr) { + switch e := expr.(type) { + case *tree.IndexedVar: + if e.Idx < len(i.ivarSeen) { + i.ivarSeen[e.Idx] = true + } + return false, expr + default: + return true, expr + } +} + +// VisitPost is a part of tree.Visitor interface. +func (i ivarExpressionVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr } diff --git a/pkg/sql/colfetcher/colbatch_scan.go b/pkg/sql/colfetcher/colbatch_scan.go index 97b265fb92e2..2988b80b5db8 100644 --- a/pkg/sql/colfetcher/colbatch_scan.go +++ b/pkg/sql/colfetcher/colbatch_scan.go @@ -206,7 +206,7 @@ func NewColBatchScan( // retrieving the hydrated immutable from cache. table := spec.BuildTableDescriptor() invertedColumn := tabledesc.FindInvertedColumn(table, spec.InvertedColumn) - tableArgs, idxMap, err := populateTableArgs( + tableArgs, _, err := populateTableArgs( ctx, flowCtx, table, table.ActiveIndexes()[spec.IndexIdx], invertedColumn, spec.Visibility, spec.HasSystemColumns, post, helper, ) @@ -214,12 +214,6 @@ func NewColBatchScan( return nil, err } - if err = keepOnlyNeededColumns( - flowCtx, tableArgs, idxMap, spec.NeededColumns, post, helper, - ); err != nil { - return nil, err - } - fetcher := cFetcherPool.Get().(*cFetcher) fetcher.cFetcherArgs = cFetcherArgs{ spec.LockingStrength, diff --git a/pkg/sql/colfetcher/index_join.go b/pkg/sql/colfetcher/index_join.go index 41d011eaaf35..bb2a948b9710 100644 --- a/pkg/sql/colfetcher/index_join.go +++ b/pkg/sql/colfetcher/index_join.go @@ -412,47 +412,13 @@ func NewColIndexJoin( // retrieving the hydrated immutable from cache. table := spec.BuildTableDescriptor() index := table.ActiveIndexes()[spec.IndexIdx] - tableArgs, idxMap, err := populateTableArgs( + tableArgs, neededColumns, err := populateTableArgs( ctx, flowCtx, table, index, nil, /* invertedCol */ spec.Visibility, spec.HasSystemColumns, post, helper, ) if err != nil { return nil, err } - if idxMap != nil { - // The index join is fetching from the primary index, so there should be - // no mapping needed. - return nil, errors.AssertionFailedf("unexpectedly non-nil idx map for the index join") - } - - // Retrieve the set of columns that the index join needs to fetch. - var neededColumns []uint32 - var neededColOrdsInWholeTable util.FastIntSet - if post.OutputColumns != nil { - neededColumns = post.OutputColumns - for _, neededColOrd := range neededColumns { - neededColOrdsInWholeTable.Add(int(neededColOrd)) - } - } else { - proc := &execinfra.ProcOutputHelper{} - // It is ok to use the evalCtx of the flowCtx here since we only use the - // ProcOutputHelper to get a set of the needed columns and will not be - // evaluating any expressions. - if err = proc.Init(post, tableArgs.typs, helper.SemaCtx, flowCtx.EvalCtx); err != nil { - return nil, err - } - neededColOrdsInWholeTable = proc.NeededColumns() - neededColumns = make([]uint32, 0, neededColOrdsInWholeTable.Len()) - for i, ok := neededColOrdsInWholeTable.Next(0); ok; i, ok = neededColOrdsInWholeTable.Next(i + 1) { - neededColumns = append(neededColumns, uint32(i)) - } - } - - if err = keepOnlyNeededColumns( - flowCtx, tableArgs, idxMap, neededColumns, post, helper, - ); err != nil { - return nil, err - } fetcher := cFetcherPool.Get().(*cFetcher) fetcher.cFetcherArgs = cFetcherArgs{ @@ -472,7 +438,7 @@ func NewColIndexJoin( } spanAssembler := colexecspan.NewColSpanAssembler( - flowCtx.Codec(), allocator, table, index, inputTypes, neededColOrdsInWholeTable, + flowCtx.Codec(), allocator, table, index, inputTypes, neededColumns, ) op := &ColIndexJoin{ diff --git a/pkg/sql/colflow/colbatch_scan_test.go b/pkg/sql/colflow/colbatch_scan_test.go index a40504a5456a..315387b04ffc 100644 --- a/pkg/sql/colflow/colbatch_scan_test.go +++ b/pkg/sql/colflow/colbatch_scan_test.go @@ -77,8 +77,7 @@ func TestColBatchScanMeta(t *testing.T) { Spans: []roachpb.Span{ td.PrimaryIndexSpan(keys.SystemSQLCodec), }, - NeededColumns: []uint32{0}, - Table: *td.TableDesc(), + Table: *td.TableDesc(), }}, ResultTypes: types.OneIntCol, } @@ -134,7 +133,6 @@ func BenchmarkColBatchScan(b *testing.B) { TableReader: &execinfrapb.TableReaderSpec{ Table: *tableDesc.TableDesc(), // Spans will be set below. - NeededColumns: []uint32{0, 1}, }}, ResultTypes: types.TwoIntCols, } diff --git a/pkg/sql/distsql_physical_planner.go b/pkg/sql/distsql_physical_planner.go index 5233131c2fc4..3ae1e633b7a6 100644 --- a/pkg/sql/distsql_physical_planner.go +++ b/pkg/sql/distsql_physical_planner.go @@ -1111,7 +1111,6 @@ func initTableReaderSpec( LockingStrength: n.lockingStrength, LockingWaitPolicy: n.lockingWaitPolicy, HasSystemColumns: n.containsSystemColumns, - NeededColumns: n.colCfg.wantedColumnsOrdinals, } if vc := getInvertedColumn(n.colCfg.invertedColumn, n.cols); vc != nil { s.InvertedColumn = vc.ColumnDesc() diff --git a/pkg/sql/distsql_spec_exec_factory.go b/pkg/sql/distsql_spec_exec_factory.go index 57a247cc170c..69292fdcd356 100644 --- a/pkg/sql/distsql_spec_exec_factory.go +++ b/pkg/sql/distsql_spec_exec_factory.go @@ -238,7 +238,6 @@ func (e *distSQLSpecExecFactory) ConstructScan( IsCheck: false, Visibility: colCfg.visibility, HasSystemColumns: scanContainsSystemColumns(&colCfg), - NeededColumns: colCfg.wantedColumnsOrdinals, } if vc := getInvertedColumn(colCfg.invertedColumn, cols); vc != nil { trSpec.InvertedColumn = vc.ColumnDesc() diff --git a/pkg/sql/execinfra/version.go b/pkg/sql/execinfra/version.go index da472445c035..774a182bf970 100644 --- a/pkg/sql/execinfra/version.go +++ b/pkg/sql/execinfra/version.go @@ -39,7 +39,7 @@ import "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" // // ATTENTION: When updating these fields, add a brief description of what // changed to the version history below. -const Version execinfrapb.DistSQLVersion = 53 +const Version execinfrapb.DistSQLVersion = 54 // MinAcceptedVersion is the oldest version that the server is compatible with. // A server will not accept flows with older versions. @@ -51,6 +51,13 @@ const MinAcceptedVersion execinfrapb.DistSQLVersion = 52 Please add new entries at the top. +- Version: 54 (MinAcceptedVersion: 52) + - Field NeededColumns has been removed from the TableReaderSpec. It was being + used for the setup of the vectorized ColBatchScans, but now the + PostProcessSpec is examined instead. This means that older gateways still + produce specs that newer remote nodes understand, thus there is no bump in + the MinAcceptedVersion. + - Version: 53 (MinAcceptedVersion: 52) - FINAL_STDDEV_POP and FINAL_VAR_POP aggregate functions were introduced to support local and final aggregation of the builtin function STDDEV_POP. It diff --git a/pkg/sql/execinfrapb/processors_sql.proto b/pkg/sql/execinfrapb/processors_sql.proto index 38202d666f1b..5c3042cfac6a 100644 --- a/pkg/sql/execinfrapb/processors_sql.proto +++ b/pkg/sql/execinfrapb/processors_sql.proto @@ -57,9 +57,8 @@ enum ScanVisibility { // columns of the rows that pass a filter expression. // // The "internal columns" of a TableReader (see ProcessorSpec) are all the -// columns of the table. Internally, only the values for the columns specified -// by needed_columns are to be populated. If is_check is set, the TableReader -// will run additional data checking procedures and the "internal columns" are: +// columns of the table. If is_check is set, the TableReader will run additional +// data checking procedures and the "internal columns" are: // - Error type (string). // - Primary key as a string, if it was obtainable. // - JSON of all decoded column values. @@ -147,10 +146,7 @@ message TableReaderSpec { // system columns in its output. optional bool has_system_columns = 14 [(gogoproto.nullable) = false]; - // Indicates the ordinals of the columns values for which are needed by the - // post-processing stage and, therefore, are to be populated. It is ignored - // if is_check is true. - repeated uint32 needed_columns = 15; + reserved 15; // Indicates an inverted column which may have a different type than the // column stored in the table descriptor. For example, the inverted column in diff --git a/pkg/sql/flowinfra/cluster_test.go b/pkg/sql/flowinfra/cluster_test.go index 236ddeafe17c..889b9b807db4 100644 --- a/pkg/sql/flowinfra/cluster_test.go +++ b/pkg/sql/flowinfra/cluster_test.go @@ -104,24 +104,21 @@ func TestClusterFlow(t *testing.T) { leafInputState := txn.GetLeafTxnInputState(ctx) tr1 := execinfrapb.TableReaderSpec{ - Table: *desc.TableDesc(), - IndexIdx: 1, - Spans: []roachpb.Span{makeIndexSpan(0, 8)}, - NeededColumns: []uint32{0, 1}, + Table: *desc.TableDesc(), + IndexIdx: 1, + Spans: []roachpb.Span{makeIndexSpan(0, 8)}, } tr2 := execinfrapb.TableReaderSpec{ - Table: *desc.TableDesc(), - IndexIdx: 1, - Spans: []roachpb.Span{makeIndexSpan(8, 12)}, - NeededColumns: []uint32{0, 1}, + Table: *desc.TableDesc(), + IndexIdx: 1, + Spans: []roachpb.Span{makeIndexSpan(8, 12)}, } tr3 := execinfrapb.TableReaderSpec{ - Table: *desc.TableDesc(), - IndexIdx: 1, - Spans: []roachpb.Span{makeIndexSpan(12, 100)}, - NeededColumns: []uint32{0, 1}, + Table: *desc.TableDesc(), + IndexIdx: 1, + Spans: []roachpb.Span{makeIndexSpan(12, 100)}, } fid := execinfrapb.FlowID{UUID: uuid.MakeV4()} diff --git a/pkg/sql/flowinfra/server_test.go b/pkg/sql/flowinfra/server_test.go index be67f7e9af29..46acac439ebf 100644 --- a/pkg/sql/flowinfra/server_test.go +++ b/pkg/sql/flowinfra/server_test.go @@ -59,11 +59,10 @@ func TestServer(t *testing.T) { td := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "t") ts := execinfrapb.TableReaderSpec{ - Table: *td.TableDesc(), - IndexIdx: 0, - Reverse: false, - Spans: []roachpb.Span{td.PrimaryIndexSpan(keys.SystemSQLCodec)}, - NeededColumns: []uint32{0, 1}, + Table: *td.TableDesc(), + IndexIdx: 0, + Reverse: false, + Spans: []roachpb.Span{td.PrimaryIndexSpan(keys.SystemSQLCodec)}, } post := execinfrapb.PostProcessSpec{ Projection: true, diff --git a/pkg/sql/opt/exec/execbuilder/testdata/aggregate b/pkg/sql/opt/exec/execbuilder/testdata/aggregate index 028d2f65b536..755c141489ee 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/aggregate +++ b/pkg/sql/opt/exec/execbuilder/testdata/aggregate @@ -727,7 +727,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /xyz/zyx/3.0/2/1 -> NULL +fetched: /xyz/zyx/?/?/1 -> NULL output row: [1] query T diff --git a/pkg/sql/opt/exec/execbuilder/testdata/ddl b/pkg/sql/opt/exec/execbuilder/testdata/ddl index 8e36c79c155e..86753e91474f 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/ddl +++ b/pkg/sql/opt/exec/execbuilder/testdata/ddl @@ -213,8 +213,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /t/b_desc/2/2 -> NULL -fetched: /t/b_desc/1/1 -> NULL +fetched: /t/b_desc/?/2 -> NULL +fetched: /t/b_desc/?/1 -> NULL fetched: /t/t_pkey/1/b/c -> /1/1 fetched: /t/t_pkey/2/b/c -> /2/2 output row: [1 1 1] diff --git a/pkg/sql/opt/exec/execbuilder/testdata/select_index b/pkg/sql/opt/exec/execbuilder/testdata/select_index index 4a64057d307d..886850993bf3 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/select_index +++ b/pkg/sql/opt/exec/execbuilder/testdata/select_index @@ -1318,7 +1318,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /noncover/b/2/1 -> NULL +fetched: /noncover/b/?/1 -> NULL fetched: /noncover/noncover_pkey/1 -> NULL fetched: /noncover/noncover_pkey/1/b -> 2 fetched: /noncover/noncover_pkey/1/c -> 3 @@ -1355,7 +1355,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /noncover/c/7 -> /5 +fetched: /noncover/c/? -> /5 fetched: /noncover/noncover_pkey/5 -> NULL fetched: /noncover/noncover_pkey/5/b -> 6 fetched: /noncover/noncover_pkey/5/c -> 7 @@ -1635,8 +1635,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /t2/bc/2/1/4 -> NULL -fetched: /t2/bc/2/3/6 -> NULL +fetched: /t2/bc/?/?/4 -> NULL +fetched: /t2/bc/?/?/6 -> NULL fetched: /t2/t2_pkey/4 -> NULL fetched: /t2/t2_pkey/4/b -> 2 fetched: /t2/t2_pkey/4/c -> 1 @@ -1657,8 +1657,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /t2/bc/2/1/4 -> NULL -fetched: /t2/bc/2/3/6 -> NULL +fetched: /t2/bc/?/?/4 -> NULL +fetched: /t2/bc/?/?/6 -> NULL fetched: /t2/t2_pkey/4 -> NULL fetched: /t2/t2_pkey/4/b -> 2 fetched: /t2/t2_pkey/4/c -> 1 @@ -1806,8 +1806,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /t4/t4_pkey/10/20 -> NULL -fetched: /t4/t4_pkey/10/20/c -> 30 +fetched: /t4/t4_pkey/?/? -> NULL +fetched: /t4/t4_pkey/?/?/c -> 30 output row: [30] # Point lookup on d does not touch the c or e families. @@ -1830,8 +1830,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY WHERE message LIKE 'fetched:%' OR message LIKE 'output row%' ORDER BY message LIKE 'fetched:%' DESC, ordinality ASC ---- -fetched: /t4/t4_pkey/10/20 -> NULL -fetched: /t4/t4_pkey/10/20/d -> 40 +fetched: /t4/t4_pkey/?/? -> NULL +fetched: /t4/t4_pkey/?/?/d -> 40 output row: [40] # Point lookup on both d and e uses a single span for the two adjacent column diff --git a/pkg/sql/physicalplan/aggregator_funcs_test.go b/pkg/sql/physicalplan/aggregator_funcs_test.go index 36683920d54d..a2534499c930 100644 --- a/pkg/sql/physicalplan/aggregator_funcs_test.go +++ b/pkg/sql/physicalplan/aggregator_funcs_test.go @@ -127,9 +127,8 @@ func checkDistAggregationInfo( makeTableReader := func(startPK, endPK int, streamID int) execinfrapb.ProcessorSpec { tr := execinfrapb.TableReaderSpec{ - Table: *tableDesc.TableDesc(), - Spans: make([]roachpb.Span, 1), - NeededColumns: []uint32{uint32(colIdx)}, + Table: *tableDesc.TableDesc(), + Spans: make([]roachpb.Span, 1), } var err error