Merge #118683

118683: sql: refactor IndexedVars and remove Used field r=mgartner a=mgartner

#### sql: remove unused "source" parameter in planner.analyzeExpr

The `source` parameter of `planner.analyzeExrp` was unused, so it has
been removed. The `planner.resolveNames` function and the
`planner.nameResolutionVisitor` field have also been removed as a
result.

Release note: None

#### schemaexpr: refactor NameResolutionVisitor

The `NameResolutionVisitor` is not used outside the `schemaexpr`
package, so it has been unexported. Additionally, it no longer attempts
to rebind indexed vars or normalized function names. Neither of these
steps are necessary because this struct is only used to resolved column
names to indexed vars for computed column and partial index expressions.

Release note: None

#### schemaexpr: rename select_name_resolution.go to name.go

Release note: None

#### sem/tree: add IndexedVarHelper.RebindTyped

This commit splits `IndexedVarHelper.Rebind` into two functions, one to
rebind untyped expressions, and one to rebind typed expressions. This is
a mechanical change that will make subsequent changes easier.

Release note: None

#### sem/tree,execinfrapb: remove ivarBinder

The `execinfrapb.ivarBinder` struct has been removed. In its place we
use the `IndexedVarHelper.Rebind` method. I believe that the special
behavior of `BindIfUnbound` where it does not bind an already bound
indexed var is vestigial and unused. Therefore, that logic is not
preserved.

Release note: None

#### sem/tree: remove IndexedVarHelper.IndexedVarUsed method

The `IndexedVarHelper.IndexedVarUsed` method was only used in tests. It
has been removed. Tests that used it now track usage of indexed vars in
their own implementation of the `IndexedVarContainer` interface.

Release note: None

#### sem/tree: remove IndexedVar.Used field

The `IndexedVar.Used` field has been removed because it was no longer
used. This shrinks the size of the `IndexedVar` struct from 40 bytes to
32 bytes, a 20% reduction. This reduces allocate memory, especially for
workloads with queries involving many columns.

Informs #117546

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>

pkg/sql/BUILD.bazel

@@ -211,7 +211,6 @@ go_library(
         "scrub_fk.go",
         "scrub_index.go",
         "scrub_unique_constraint.go",
-        "select_name_resolution.go",
         "sequence.go",
         "sequence_select.go",
         "serial.go",

pkg/sql/alter_role.go

@@ -415,7 +415,7 @@ func (p *planner) processSetOrResetClause(
 		expr = paramparse.UnresolvedNameToStrVal(expr)
 
 		typedValue, err := p.analyzeExpr(
-			ctx, expr, nil, tree.IndexedVarHelper{}, types.String, false, "ALTER ROLE ... SET ",
+			ctx, expr, tree.IndexedVarHelper{}, types.String, false, "ALTER ROLE ... SET ",
 		)
 		if err != nil {
 			return unknown, "", sessionVar{}, nil, wrapSetVarError(err, varName, expr.String())

pkg/sql/alter_table.go

@@ -122,7 +122,6 @@ func (p *planner) AlterTable(ctx context.Context, n *tree.AlterTable) (planNode,
 		}
 		typedExpr, err := p.analyzeExpr(
 			ctx, injectStats.Stats,
-			nil, /* sources - no name resolution */
 			tree.IndexedVarHelper{},
 			types.Jsonb, true, /* requireType */
 			"INJECT STATISTICS" /* typingContext */)

pkg/sql/analyze_expr.go

@@ -13,46 +13,32 @@ package sql
 import (
 	"context"
 
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 )
 
 // analyzeExpr performs semantic analysis of an expression, including:
-// - replacing sub-queries by a sql.subquery node;
-// - resolving names (optional);
 // - type checking (with optional type enforcement);
 // - normalization.
 // The parameters sources and IndexedVars, if both are non-nil, indicate
 // name resolution should be performed. The IndexedVars map will be filled
 // as a result.
 func (p *planner) analyzeExpr(
 	ctx context.Context,
-	raw tree.Expr,
-	source *colinfo.DataSourceInfo,
+	expr tree.Expr,
 	iVarHelper tree.IndexedVarHelper,
 	expectedType *types.T,
 	requireType bool,
 	typingContext string,
 ) (tree.TypedExpr, error) {
-	// Perform optional name resolution.
-	resolved := raw
-	if source != nil {
-		var err error
-		resolved, err = p.resolveNames(raw, source, iVarHelper)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	// Type check.
 	var typedExpr tree.TypedExpr
 	var err error
 	p.semaCtx.IVarContainer = iVarHelper.Container()
 	if requireType {
-		typedExpr, err = tree.TypeCheckAndRequire(ctx, resolved, &p.semaCtx, expectedType, typingContext)
+		typedExpr, err = tree.TypeCheckAndRequire(ctx, expr, &p.semaCtx, expectedType, typingContext)
 	} else {
-		typedExpr, err = tree.TypeCheck(ctx, resolved, &p.semaCtx, expectedType)
+		typedExpr, err = tree.TypeCheck(ctx, expr, &p.semaCtx, expectedType)
 	}
 	p.semaCtx.IVarContainer = nil
 	if err != nil {

pkg/sql/catalog/schemaexpr/BUILD.bazel

@@ -12,8 +12,8 @@ go_library(
         "doc.go",
         "expr.go",
         "hash_sharded_compute_expr.go",
+        "name.go",
         "partial_index.go",
-        "select_name_resolution.go",
         "sequence_options.go",
         "unique_contraint.go",
     ],

pkg/sql/catalog/schemaexpr/expr.go

@@ -363,8 +363,14 @@ func newNameResolver(
 // resolveNames returns an expression equivalent to the input expression with
 // unresolved names replaced with IndexedVars.
 func (nr *nameResolver) resolveNames(expr tree.Expr) (tree.Expr, error) {
-	var v NameResolutionVisitor
-	return ResolveNamesUsingVisitor(&v, expr, nr.source, *nr.ivarHelper)
+	v := nameResolutionVisitor{
+		iVarHelper: *nr.ivarHelper,
+		resolver: colinfo.ColumnResolver{
+			Source: nr.source,
+		},
+	}
+	expr, _ = tree.WalkExpr(&v, expr)
+	return expr, v.err
 }
 
 // addColumn adds a new column to the nameResolver so that it can be resolved in

pkg/sql/catalog/schemaexpr/name.go

@@ -0,0 +1,63 @@
+// Copyright 2016 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+//
+// This file implements the select code that deals with column references
+// and resolving column names in expressions.
+
+package schemaexpr
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+// nameResolutionVisitor is a tree.Visitor implementation used to resolve column
+// names in an expression as indexed vars.
+type nameResolutionVisitor struct {
+	err        error
+	iVarHelper tree.IndexedVarHelper
+	resolver   colinfo.ColumnResolver
+}
+
+var _ tree.Visitor = &nameResolutionVisitor{}
+
+// VisitPre implements tree.Visitor.
+func (v *nameResolutionVisitor) VisitPre(expr tree.Expr) (recurse bool, newNode tree.Expr) {
+	if v.err != nil {
+		return false, expr
+	}
+
+	switch t := expr.(type) {
+	case *tree.UnresolvedName:
+		vn, err := t.NormalizeVarName()
+		if err != nil {
+			v.err = err
+			return false, expr
+		}
+		return v.VisitPre(vn)
+
+	case *tree.ColumnItem:
+		_, err := colinfo.ResolveColumnItem(context.Background(), &v.resolver, t)
+		if err != nil {
+			v.err = err
+			return false, expr
+		}
+
+		colIdx := v.resolver.ResolverState.ColIdx
+		ivar := v.iVarHelper.IndexedVar(colIdx)
+		return true, ivar
+	}
+	return true, expr
+}
+
+// VisitPost implements tree.Visitor.
+func (*nameResolutionVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr }

pkg/sql/distsql_plan_stats.go

@@ -505,7 +505,7 @@ func (dsp *DistSQLPlanner) createStatsPlan(
 		var rb renderBuilder
 		rb.init(exec.Node(planNode(&scan)), exec.OutputOrdering{})
 		for i, expr := range exprs {
-			exprs[i] = rb.r.ivarHelper.Rebind(expr)
+			exprs[i] = rb.r.ivarHelper.RebindTyped(expr)
 		}
 		rb.setOutput(exprs, resultCols)
 

pkg/sql/execinfrapb/expr.go

@@ -26,30 +26,6 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// ivarBinder is a tree.Visitor that binds ordinal references
-// (IndexedVars represented by @1, @2, ...) to an IndexedVarContainer.
-type ivarBinder struct {
-	h   *tree.IndexedVarHelper
-	err error
-}
-
-func (v *ivarBinder) VisitPre(expr tree.Expr) (recurse bool, newExpr tree.Expr) {
-	if v.err != nil {
-		return false, expr
-	}
-	if ivar, ok := expr.(*tree.IndexedVar); ok {
-		newVar, err := v.h.BindIfUnbound(ivar)
-		if err != nil {
-			v.err = err
-			return false, expr
-		}
-		return false, newVar
-	}
-	return true, expr
-}
-
-func (*ivarBinder) VisitPost(expr tree.Expr) tree.Expr { return expr }
-
 // DeserializeExpr deserializes expr, binds the indexed variables to the
 // provided IndexedVarHelper, and evaluates any constants in the expression.
 //
@@ -130,12 +106,8 @@ func processExpression(
 		return nil, err
 	}
 
-	// Bind IndexedVars to our eh.Vars.
-	v := ivarBinder{h: h, err: nil}
-	expr, _ = tree.WalkExpr(&v, expr)
-	if v.err != nil {
-		return nil, v.err
-	}
+	// Bind IndexedVars to h.
+	expr = h.Rebind(expr)
 
 	semaCtx.IVarContainer = h.Container()
 	// Convert to a fully typed expression.
@@ -355,7 +327,7 @@ func (eh *exprHelper) prepareExpr(ctx context.Context, expr Expression) (tree.Ty
 		return nil, nil
 	}
 	if expr.LocalExpr != nil {
-		eh.vars.Rebind(expr.LocalExpr)
+		eh.vars.RebindTyped(expr.LocalExpr)
 		return expr.LocalExpr, nil
 	}
 	return DeserializeExpr(ctx, expr.Expr, eh.semaCtx, eh.evalCtx, &eh.vars)