sql: correct the usage of UDF terms "arguments" and "parameters" #92758

mgartner · 2022-11-30T17:59:21Z

sql: correct the usage of UDF terms "arguments" and "parameters"

A parameter is a variable in a function definition. An argument is
an expression passed to a function when the function is invoked.
UDF-related code was not using this nomenclature correctly, creating
confusion. This commit fixes up our usage of these terms within SQL
optimization and execution. I know this is a pedantic change, but it is
motivated by my struggles in writing clear code and comments with the
previously overloading of the term argument.

Epic: CRDB-20370

Release note: None

sql: correct the usage of terms "arguments" and "parameters" in descriptors

Release note: None

cockroach-teamcity · 2022-11-30T17:59:32Z

This change is

mgartner · 2022-11-30T18:01:11Z

The first commit is mostly SQL Queries related code and the second commit is mostly SQL Schema related code plus the parser changes.

chengxiong-ruan

LGTM. Thanks for doing this. This is very important!

A _parameter_ is a variable in a function definition. An _argument_ is an expression passed to a function when the function is invoked. UDF-related code was not using this nomenclature correctly, creating confusion. This commit fixes up our usage of these terms within SQL optimization and execution. I know this is a pedantic change, but it is motivated by my struggles in writing clear code and comments with the previously overloading of the term _argument_. Release note: None

DrewKimball

Left a bunch of minor comments, but feel free to pick and choose whichever ones you think matter for clarity.

Reviewed 11 of 11 files at r1, 21 of 21 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

pkg/sql/create_function.go line 231 at r2 (raw file):

) (fnDesc *funcdesc.Mutable, isNew bool, err error) {
	// Resolve argument types.
	argTypes := make([]*types.T, len(n.cf.Params))

argTypes->paramTypes?

pkg/sql/create_function.go line 244 at r2 (raw file):

			argNameSeen[param.Name] = struct{}{}
		}
		pbArg, err := makeFunctionArg(params.ctx, param, params.p)

pbArg->pbParam

makeFunctionArg is only used here, so it could be renamed too.

pkg/sql/create_function.go line 454 at r2 (raw file):

func makeFunctionArg(
	ctx context.Context, arg tree.FuncParam, typeResolver tree.TypeReferenceResolver,

[nit] could rename arg to param

pkg/sql/create_function.go line 460 at r2 (raw file):

	}
	var err error
	pbParam.Class, err = funcdesc.ArgClassToProto(arg.Class)

ArgClassToProto is only used here, could rename it.

pkg/sql/pg_catalog.go line 2424 at r2 (raw file):

						isStrict := fnDesc.GetNullInputBehavior() != catpb.Function_CALLED_ON_NULL_INPUT
						argTypes := tree.NewDArray(types.Oid)
						argModes := tree.NewDArray(types.String)

A couple more places here where we're using arg instead of param:
argTypes->paramTypes
argModes->paramModes
argNames->paramNames
argNamesArray->paramNamesArray
foundAnyArgNames->foundAnyParamNames

pkg/sql/catalog/funcdesc/func_desc.go line 555 at r2 (raw file):

	}

	argTypes := make(tree.ArgTypes, 0, len(desc.Params))

argTypes->paramTypes
We could even change tree.ArgTypes since it only seems to be used here and one other place for functions, though that might be too far.

pkg/sql/catalog/funcdesc/func_desc.go line 676 at r2 (raw file):

}

func toTreeNodeArgClass(class catpb.Function_Param_Class) tree.FuncParamClass {

This is only used in one place, so it's name can be easily changed.

pkg/sql/delegate/show_grants.go line 261 at r2 (raw file):

				return nil, err
			}
			argTypes, err := fn.ParamTypes(d.ctx, d.catalog)

[nit] argTypes -> paramTypes

pkg/sql/opt/exec/execbuilder/scalar.go line 677 at r1 (raw file):

	// represents. If the column does not represent an argument, then ok=false
	// is returned.
	argOrd := func(col opt.ColumnID) (ord int, ok bool) {

Does it make sense to say that the column ID represents an argument, rather than a parameter?

pkg/sql/opt/exec/execbuilder/scalar.go line 703 at r1 (raw file):

		stmt := udf.Body[stmtIdx]

		// Copy the expression into a new memo. Replace argument references with

Should it be 'parameter references' rather than 'argument references'? The idea is to replace parameter references within the function body with resolved arguments, right?

pkg/sql/opt/optbuilder/scalar.go line 632 at r1 (raw file):

	// start with an empty scope because a statement in the function body cannot
	// refer to anything from the outer expression. If there are function
	// arguments, we add them as columns to the scope so that references to them

[nit] 'function arguments' -> 'function parameters'

pkg/sql/opt/optbuilder/scope_column.go line 122 at r1 (raw file):

const maxFuncParams = 100

// funcParamOrd is a 1-based ordinal of a function parameters.

[nit] parameters->parameter

pkg/sql/opt/optbuilder/scope_column.go line 138 at r1 (raw file):

}

// funcParamReferencedBy returns true if the scopeColumn is a function argument

argument->parameter

pkg/sql/opt/testutils/testcat/function.go line 72 at r2 (raw file):

	// Resolve the parameter names and types.
	argTypes := make(tree.ArgTypes, len(c.Params))

argTypes->paramTypes

mgartner

Thanks for the thorough review! I made a bunch of changes, so please take another look.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @benbardin, @DrewKimball, and @HonoreDB)

pkg/sql/create_function.go line 231 at r2 (raw file):