From cee2f348e50bf2e75a22e2d94d4dbe9387faba8f Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 13 Jul 2022 11:55:44 -0400 Subject: [PATCH 1/3] sql/sem/builtins: move builtins definition to a new, registry package Previously, the definition of builtin functions live in the `builtins` package. This was undesirable because various other packages need to acceess builtins properties by name, but it has a been a headache to achieve this without importing the `builtins` package, which stands pretty high in the dependecy chain (e.g. `seqexpr`, `memo`). This PR moves builtins definition into a new registry package that the `builtins` package calls to register builtin functions, which happens in the `init()` function. This way, other lower level packages, who wish to access builtins properties, need only to import the newly created `builtinsregistry` package. Release note: None --- pkg/cmd/docgen/BUILD.bazel | 1 + pkg/cmd/docgen/funcs.go | 3 +- pkg/sql/BUILD.bazel | 2 + pkg/sql/builtin_test.go | 3 +- pkg/sql/catalog/seqexpr/BUILD.bazel | 1 + pkg/sql/catalog/seqexpr/sequence_test.go | 9 +- pkg/sql/crdb_internal.go | 3 +- pkg/sql/create_view.go | 6 +- pkg/sql/distsql/BUILD.bazel | 2 +- pkg/sql/distsql/columnar_operators_test.go | 4 +- pkg/sql/execinfra/execagg/BUILD.bazel | 1 + pkg/sql/execinfra/execagg/base.go | 5 +- pkg/sql/opt/exec/execbuilder/BUILD.bazel | 2 +- pkg/sql/opt/exec/execbuilder/relational.go | 4 +- pkg/sql/opt/exec/execbuilder/scalar.go | 4 +- pkg/sql/opt/memo/BUILD.bazel | 1 + pkg/sql/opt/memo/typing.go | 2 +- pkg/sql/opt/memo/typing_test.go | 3 +- pkg/sql/opt/norm/BUILD.bazel | 1 + pkg/sql/opt/norm/comp_funcs.go | 4 +- pkg/sql/opt/norm/factory.go | 5 +- pkg/sql/opt/optbuilder/BUILD.bazel | 2 +- pkg/sql/opt/optbuilder/create_table.go | 4 +- pkg/sql/opt/optbuilder/scalar.go | 4 +- pkg/sql/pg_catalog.go | 7 +- pkg/sql/rename_database.go | 4 +- pkg/sql/row/BUILD.bazel | 2 +- pkg/sql/row/expr_walker.go | 4 +- pkg/sql/schemachanger/scbuild/BUILD.bazel | 2 +- .../schemachanger/scbuild/builder_state.go | 6 +- pkg/sql/schemachanger/scdecomp/BUILD.bazel | 2 +- pkg/sql/schemachanger/scdecomp/helpers.go | 4 +- .../scexec/scmutationexec/BUILD.bazel | 2 +- .../scexec/scmutationexec/helpers.go | 4 +- pkg/sql/sem/builtins/BUILD.bazel | 2 + pkg/sql/sem/builtins/aggregate_builtins.go | 5 +- pkg/sql/sem/builtins/all_builtins.go | 92 +++++++++---------- pkg/sql/sem/builtins/all_builtins_test.go | 13 +-- pkg/sql/sem/builtins/builtins.go | 19 ++-- pkg/sql/sem/builtins/builtins_test.go | 13 ++- .../sem/builtins/builtinsregistry/BUILD.bazel | 9 ++ .../builtinsregistry/builtins_registry.go | 56 +++++++++++ pkg/sql/sem/builtins/generator_builtins.go | 7 +- .../sem/builtins/generator_probe_ranges.go | 7 +- pkg/sql/sem/builtins/geo_builtins.go | 5 +- pkg/sql/sem/builtins/help_test.go | 8 +- pkg/sql/sem/builtins/math_builtins.go | 5 +- pkg/sql/sem/builtins/overlaps_builtins.go | 5 +- pkg/sql/sem/builtins/pg_builtins.go | 17 ++-- pkg/sql/sem/builtins/pgcrypto_builtins.go | 5 +- pkg/sql/sem/builtins/replication_builtins.go | 5 +- pkg/sql/sem/builtins/trigram_builtins.go | 5 +- pkg/sql/sem/builtins/window_builtins.go | 6 +- pkg/sql/sem/tree/function_definition.go | 4 + pkg/sql/sequence.go | 5 +- pkg/sql/tests/BUILD.bazel | 1 + pkg/sql/tests/rsg_test.go | 3 +- pkg/upgrade/upgrades/BUILD.bazel | 2 +- ...upgrade_sequence_to_be_referenced_by_ID.go | 10 +- 59 files changed, 237 insertions(+), 185 deletions(-) create mode 100644 pkg/sql/sem/builtins/builtinsregistry/BUILD.bazel create mode 100644 pkg/sql/sem/builtins/builtinsregistry/builtins_registry.go diff --git a/pkg/cmd/docgen/BUILD.bazel b/pkg/cmd/docgen/BUILD.bazel index a05f13ac19dd..0a56ad014fc2 100644 --- a/pkg/cmd/docgen/BUILD.bazel +++ b/pkg/cmd/docgen/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//pkg/cli/exit", "//pkg/cmd/docgen/extract", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/tree", "//pkg/util/envutil", "//pkg/util/log", diff --git a/pkg/cmd/docgen/funcs.go b/pkg/cmd/docgen/funcs.go index 2aa61167b78d..ac17671ef48c 100644 --- a/pkg/cmd/docgen/funcs.go +++ b/pkg/cmd/docgen/funcs.go @@ -21,6 +21,7 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/errors" "github.com/golang-commonmark/markdown" @@ -181,7 +182,7 @@ func generateFunctions(from []string, categorize bool) []byte { continue } seen[name] = struct{}{} - props, fns := builtins.GetBuiltinProperties(name) + props, fns := builtinsregistry.GetBuiltinProperties(name) if !props.ShouldDocument() { continue } diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index e4d7c0569347..c22500259785 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -392,6 +392,7 @@ go_library( "//pkg/sql/sem/asof", "//pkg/sql/sem/builtins", "//pkg/sql/sem/builtins/builtinconstants", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/cast", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", @@ -690,6 +691,7 @@ go_test( "//pkg/sql/scrub", "//pkg/sql/scrub/scrubtestutils", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", "//pkg/sql/sem/eval", diff --git a/pkg/sql/builtin_test.go b/pkg/sql/builtin_test.go index f3e5485d20c7..fd1524c9dde5 100644 --- a/pkg/sql/builtin_test.go +++ b/pkg/sql/builtin_test.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -46,7 +47,7 @@ func TestFuncNull(t *testing.T) { case "crdb_internal.force_panic", "crdb_internal.force_log_fatal", "pg_sleep": continue } - _, variations := builtins.GetBuiltinProperties(name) + _, variations := builtinsregistry.GetBuiltinProperties(name) for _, builtin := range variations { // Untyped NULL. { diff --git a/pkg/sql/catalog/seqexpr/BUILD.bazel b/pkg/sql/catalog/seqexpr/BUILD.bazel index 45d375bceb61..7dd146cab12c 100644 --- a/pkg/sql/catalog/seqexpr/BUILD.bazel +++ b/pkg/sql/catalog/seqexpr/BUILD.bazel @@ -22,6 +22,7 @@ go_test( ":seqexpr", "//pkg/sql/parser", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/tree", "//pkg/sql/types", ], diff --git a/pkg/sql/catalog/seqexpr/sequence_test.go b/pkg/sql/catalog/seqexpr/sequence_test.go index 4414c51f5fd6..19f14348a150 100644 --- a/pkg/sql/catalog/seqexpr/sequence_test.go +++ b/pkg/sql/catalog/seqexpr/sequence_test.go @@ -17,7 +17,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr" "github.com/cockroachdb/cockroach/pkg/sql/parser" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + _ "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" // register all builtins in builtins:init() for seqexpr package + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -50,7 +51,7 @@ func TestGetSequenceFromFunc(t *testing.T) { if !ok { t.Fatal("Expr is not a FuncExpr") } - identifier, err := seqexpr.GetSequenceFromFunc(funcExpr, builtins.GetBuiltinProperties) + identifier, err := seqexpr.GetSequenceFromFunc(funcExpr, builtinsregistry.GetBuiltinProperties) if err != nil { t.Fatal(err) } @@ -96,7 +97,7 @@ func TestGetUsedSequences(t *testing.T) { if err != nil { t.Fatal(err) } - identifiers, err := seqexpr.GetUsedSequences(typedExpr, builtins.GetBuiltinProperties) + identifiers, err := seqexpr.GetUsedSequences(typedExpr, builtinsregistry.GetBuiltinProperties) if err != nil { t.Fatal(err) } @@ -147,7 +148,7 @@ func TestReplaceSequenceNamesWithIDs(t *testing.T) { if err != nil { t.Fatal(err) } - newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(typedExpr, namesToID, builtins.GetBuiltinProperties) + newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(typedExpr, namesToID, builtinsregistry.GetBuiltinProperties) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 91d44d1b7949..504beedfd496 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -58,6 +58,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/rowenc" "github.com/cockroachdb/cockroach/pkg/sql/rowinfra" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -2412,7 +2413,7 @@ CREATE TABLE crdb_internal.builtin_functions ( )`, populate: func(ctx context.Context, _ *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { for _, name := range builtins.AllBuiltinNames { - props, overloads := builtins.GetBuiltinProperties(name) + props, overloads := builtinsregistry.GetBuiltinProperties(name) for _, f := range overloads { if err := addRow( tree.NewDString(name), diff --git a/pkg/sql/create_view.go b/pkg/sql/create_view.go index 14067c15ed62..6a5ab30ca057 100644 --- a/pkg/sql/create_view.go +++ b/pkg/sql/create_view.go @@ -33,7 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" "github.com/cockroachdb/cockroach/pkg/sql/privilege" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" @@ -427,7 +427,7 @@ func replaceSeqNamesWithIDs( ctx context.Context, sc resolver.SchemaResolver, viewQuery string, ) (string, error) { replaceSeqFunc := func(expr tree.Expr) (recurse bool, newExpr tree.Expr, err error) { - seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtinsregistry.GetBuiltinProperties) if err != nil { return false, expr, err } @@ -439,7 +439,7 @@ func replaceSeqNamesWithIDs( } seqNameToID[seqIdentifier.SeqName] = int64(seqDesc.ID) } - newExpr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtins.GetBuiltinProperties) + newExpr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtinsregistry.GetBuiltinProperties) if err != nil { return false, expr, err } diff --git a/pkg/sql/distsql/BUILD.bazel b/pkg/sql/distsql/BUILD.bazel index 3208cee3b020..0b4775be6d71 100644 --- a/pkg/sql/distsql/BUILD.bazel +++ b/pkg/sql/distsql/BUILD.bazel @@ -78,7 +78,7 @@ go_test( "//pkg/sql/randgen", "//pkg/sql/rowenc", "//pkg/sql/rowexec", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/types", diff --git a/pkg/sql/distsql/columnar_operators_test.go b/pkg/sql/distsql/columnar_operators_test.go index 2d97ceb00ded..13cd6bb1a3be 100644 --- a/pkg/sql/distsql/columnar_operators_test.go +++ b/pkg/sql/distsql/columnar_operators_test.go @@ -29,7 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/randgen" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -121,7 +121,7 @@ func TestAggregateFuncToNumArguments(t *testing.T) { check := func(t *testing.T, fn execinfrapb.AggregatorSpec_Func) { n, ok := aggregateFuncToNumArguments[fn] require.Truef(t, ok, "didn't find number of arguments for %s", fn) - _, overloads := builtins.GetBuiltinProperties(strings.ToLower(fn.String())) + _, overloads := builtinsregistry.GetBuiltinProperties(strings.ToLower(fn.String())) checkForOverload(t, n, overloads) } diff --git a/pkg/sql/execinfra/execagg/BUILD.bazel b/pkg/sql/execinfra/execagg/BUILD.bazel index 608e5097ac7c..fde25b0e64c9 100644 --- a/pkg/sql/execinfra/execagg/BUILD.bazel +++ b/pkg/sql/execinfra/execagg/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//pkg/sql/execinfrapb", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/types", diff --git a/pkg/sql/execinfra/execagg/base.go b/pkg/sql/execinfra/execagg/base.go index daf6ed3cbc5d..40511eb3a725 100644 --- a/pkg/sql/execinfra/execagg/base.go +++ b/pkg/sql/execinfra/execagg/base.go @@ -15,6 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -38,7 +39,7 @@ func GetAggregateInfo( return builtins.NewAnyNotNullAggregate, inputTypes[0], nil } - _, builtins := builtins.GetBuiltinProperties(strings.ToLower(fn.String())) + _, builtins := builtinsregistry.GetBuiltinProperties(strings.ToLower(fn.String())) for _, b := range builtins { typs := b.Types.Types() if len(typs) != len(inputTypes) { @@ -131,7 +132,7 @@ func GetWindowFunctionInfo( "function is neither an aggregate nor a window function", ) } - _, builtins := builtins.GetBuiltinProperties(strings.ToLower(funcStr)) + _, builtins := builtinsregistry.GetBuiltinProperties(strings.ToLower(funcStr)) for _, b := range builtins { typs := b.Types.Types() if len(typs) != len(inputTypes) { diff --git a/pkg/sql/opt/exec/execbuilder/BUILD.bazel b/pkg/sql/opt/exec/execbuilder/BUILD.bazel index 6db700a483ce..3960e181acf4 100644 --- a/pkg/sql/opt/exec/execbuilder/BUILD.bazel +++ b/pkg/sql/opt/exec/execbuilder/BUILD.bazel @@ -33,7 +33,7 @@ go_library( "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "//pkg/sql/row", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/sem/tree/treebin", diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 482746dd2e41..8fa1e53620f9 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -30,7 +30,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/xform" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treewindow" @@ -2404,7 +2404,7 @@ func (b *Builder) buildWindow(w *memo.WindowExpr) (execPlan, error) { if !b.disableTelemetry { telemetry.Inc(sqltelemetry.WindowFunctionCounter(name)) } - props, _ := builtins.GetBuiltinProperties(name) + props, _ := builtinsregistry.GetBuiltinProperties(name) args := make([]tree.TypedExpr, fn.ChildCount()) argIdxs[i] = make([]exec.NodeColumnOrdinal, fn.ChildCount()) diff --git a/pkg/sql/opt/exec/execbuilder/scalar.go b/pkg/sql/opt/exec/execbuilder/scalar.go index 7c79a949c95f..f54689a804c3 100644 --- a/pkg/sql/opt/exec/execbuilder/scalar.go +++ b/pkg/sql/opt/exec/execbuilder/scalar.go @@ -14,7 +14,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/exec" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treebin" @@ -362,7 +362,7 @@ func (b *Builder) buildAssignmentCast( } const fnName = "crdb_internal.assignment_cast" funcRef := b.wrapFunction(fnName) - props, overloads := builtins.GetBuiltinProperties(fnName) + props, overloads := builtinsregistry.GetBuiltinProperties(fnName) return tree.NewTypedFuncExpr( funcRef, 0, /* aggQualifier */ diff --git a/pkg/sql/opt/memo/BUILD.bazel b/pkg/sql/opt/memo/BUILD.bazel index d44105c48479..d70811bcd77f 100644 --- a/pkg/sql/opt/memo/BUILD.bazel +++ b/pkg/sql/opt/memo/BUILD.bazel @@ -90,6 +90,7 @@ go_test( "//pkg/sql/parser", "//pkg/sql/randgen", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/sem/tree/treewindow", diff --git a/pkg/sql/opt/memo/typing.go b/pkg/sql/opt/memo/typing.go index a9ba20ebdaeb..8e9a35761cf9 100644 --- a/pkg/sql/opt/memo/typing.go +++ b/pkg/sql/opt/memo/typing.go @@ -97,7 +97,7 @@ func BinaryAllowsNullArgs(op opt.Operator, leftType, rightType *types.T) bool { return o.NullableArgs } -// GetBuiltinProperties is set to builtins.GetBuiltinProperties in an init +// GetBuiltinProperties is set to builtinsregistry.GetBuiltinProperties in an init // function in the norm package. This allows the memo package to resolve builtin // functions without importing the builtins package. var GetBuiltinProperties func(name string) (*tree.FunctionProperties, []tree.Overload) diff --git a/pkg/sql/opt/memo/typing_test.go b/pkg/sql/opt/memo/typing_test.go index c0bce5b8fb17..6a7ac82956be 100644 --- a/pkg/sql/opt/memo/typing_test.go +++ b/pkg/sql/opt/memo/typing_test.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" tu "github.com/cockroachdb/cockroach/pkg/testutils" @@ -137,7 +138,7 @@ func TestTypingAggregateAssumptions(t *testing.T) { // These are treated as special cases. continue } - _, overloads := builtins.GetBuiltinProperties(name) + _, overloads := builtinsregistry.GetBuiltinProperties(name) for i, overload := range overloads { // Check for basic ambiguity where two different aggregate function // overloads both allow equivalent operand types. diff --git a/pkg/sql/opt/norm/BUILD.bazel b/pkg/sql/opt/norm/BUILD.bazel index b7ab50eef318..b0a5a9bcc49a 100644 --- a/pkg/sql/opt/norm/BUILD.bazel +++ b/pkg/sql/opt/norm/BUILD.bazel @@ -42,6 +42,7 @@ go_library( "//pkg/sql/privilege", "//pkg/sql/rowenc/keyside", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/cast", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", diff --git a/pkg/sql/opt/norm/comp_funcs.go b/pkg/sql/opt/norm/comp_funcs.go index b8a260906ff3..b5a11a8ec42b 100644 --- a/pkg/sql/opt/norm/comp_funcs.go +++ b/pkg/sql/opt/norm/comp_funcs.go @@ -13,7 +13,7 @@ package norm import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/errors" @@ -104,7 +104,7 @@ func (c *CustomFuncs) MakeTimeZoneFunction(zone opt.ScalarExpr, ts opt.ScalarExp // timezone() function with a second argument that matches the given input type. // If no overload is found, findTimeZoneFunction panics. func findTimeZoneFunction(typ *types.T) (*tree.FunctionProperties, *tree.Overload) { - props, overloads := builtins.GetBuiltinProperties("timezone") + props, overloads := builtinsregistry.GetBuiltinProperties("timezone") for o := range overloads { overload := &overloads[o] if overload.Types.MatchAt(typ, 1) { diff --git a/pkg/sql/opt/norm/factory.go b/pkg/sql/opt/norm/factory.go index 3bbc102003f2..b9c3029c70b8 100644 --- a/pkg/sql/opt/norm/factory.go +++ b/pkg/sql/opt/norm/factory.go @@ -15,7 +15,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + _ "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" // register all builtins in builtins:init() for memo package + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -109,7 +110,7 @@ const maxConstructorStackDepth = 10_000 // Injecting this builtins dependency in the init function allows the memo // package to access builtin properties without importing the builtins package. func init() { - memo.GetBuiltinProperties = builtins.GetBuiltinProperties + memo.GetBuiltinProperties = builtinsregistry.GetBuiltinProperties } // Init initializes a Factory structure with a new, blank memo structure inside. diff --git a/pkg/sql/opt/optbuilder/BUILD.bazel b/pkg/sql/opt/optbuilder/BUILD.bazel index c149ba6b786c..a686333a03cf 100644 --- a/pkg/sql/opt/optbuilder/BUILD.bazel +++ b/pkg/sql/opt/optbuilder/BUILD.bazel @@ -69,7 +69,7 @@ go_library( "//pkg/sql/pgwire/pgerror", "//pkg/sql/privilege", "//pkg/sql/sem/asof", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/cast", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/eval", diff --git a/pkg/sql/opt/optbuilder/create_table.go b/pkg/sql/opt/optbuilder/create_table.go index 0093de71a34e..3efb5c057660 100644 --- a/pkg/sql/opt/optbuilder/create_table.go +++ b/pkg/sql/opt/optbuilder/create_table.go @@ -13,7 +13,7 @@ package optbuilder import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -81,7 +81,7 @@ func (b *Builder) buildCreateTable(ct *tree.CreateTable, inScope *scope) (outSco input = outScope.expr if !ct.AsHasUserSpecifiedPrimaryKey() { // Synthesize rowid column, and append to end of column list. - props, overloads := builtins.GetBuiltinProperties("unique_rowid") + props, overloads := builtinsregistry.GetBuiltinProperties("unique_rowid") private := &memo.FunctionPrivate{ Name: "unique_rowid", Typ: types.Int, diff --git a/pkg/sql/opt/optbuilder/scalar.go b/pkg/sql/opt/optbuilder/scalar.go index bd3fffa2a668..c2ed9e21d6f2 100644 --- a/pkg/sql/opt/optbuilder/scalar.go +++ b/pkg/sql/opt/optbuilder/scalar.go @@ -24,7 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treebin" @@ -556,7 +556,7 @@ func (b *Builder) buildFunction( // Add a dependency on sequences that are used as a string argument. if b.trackViewDeps { - seqIdentifier, err := seqexpr.GetSequenceFromFunc(f, builtins.GetBuiltinProperties) + seqIdentifier, err := seqexpr.GetSequenceFromFunc(f, builtinsregistry.GetBuiltinProperties) if err != nil { panic(err) } diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index b54983178f7c..d094f27198a0 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -35,6 +35,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/cast" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" @@ -2272,7 +2273,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-proc.html`, if unicode.IsUpper(first) { continue } - props, overloads := builtins.GetBuiltinProperties(name) + props, overloads := builtinsregistry.GetBuiltinProperties(name) isAggregate := props.Class == tree.AggregateClass isWindow := props.Class == tree.WindowClass for _, builtin := range overloads { @@ -4149,7 +4150,7 @@ https://www.postgresql.org/docs/9.6/catalog-pg-aggregate.html`, // any_not_null is treated as a special case. continue } - _, overloads := builtins.GetBuiltinProperties(name) + _, overloads := builtinsregistry.GetBuiltinProperties(name) for _, overload := range overloads { params, _ := tree.GetParamsAndReturnType(overload) sortOperatorOid := oidZero @@ -4444,7 +4445,7 @@ func (h oidHasher) BuiltinOid(name string, builtin *tree.Overload) *tree.DOid { } func (h oidHasher) RegProc(name string) tree.Datum { - _, overloads := builtins.GetBuiltinProperties(name) + _, overloads := builtinsregistry.GetBuiltinProperties(name) if len(overloads) == 0 { return tree.DNull } diff --git a/pkg/sql/rename_database.go b/pkg/sql/rename_database.go index 010f2e338440..8df2f57d2aa6 100644 --- a/pkg/sql/rename_database.go +++ b/pkg/sql/rename_database.go @@ -23,7 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util" @@ -298,7 +298,7 @@ func isAllowedDependentDescInRenameDatabase( if err != nil { return false, "", err } - seqIdentifiers, err := seqexpr.GetUsedSequences(typedExpr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(typedExpr, builtinsregistry.GetBuiltinProperties) if err != nil { return false, "", err } diff --git a/pkg/sql/row/BUILD.bazel b/pkg/sql/row/BUILD.bazel index d36ef3d2b719..d6f9e9383a4e 100644 --- a/pkg/sql/row/BUILD.bazel +++ b/pkg/sql/row/BUILD.bazel @@ -51,8 +51,8 @@ go_library( "//pkg/sql/rowenc/valueside", "//pkg/sql/rowinfra", "//pkg/sql/scrub", - "//pkg/sql/sem/builtins", "//pkg/sql/sem/builtins/builtinconstants", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/transform", "//pkg/sql/sem/tree", diff --git a/pkg/sql/row/expr_walker.go b/pkg/sql/row/expr_walker.go index f961e093415d..ffef6a99f6e2 100644 --- a/pkg/sql/row/expr_walker.go +++ b/pkg/sql/row/expr_walker.go @@ -25,8 +25,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" @@ -629,7 +629,7 @@ var supportedImportFuncOverrides = map[string]*customFunc{ visitorSideEffect: func(annot *tree.Annotations, fn *tree.FuncExpr) error { // Get sequence name so that we can update the annotation with the number // of nextval calls to this sequence in a row. - seqIdentifier, err := seqexpr.GetSequenceFromFunc(fn, builtins.GetBuiltinProperties) + seqIdentifier, err := seqexpr.GetSequenceFromFunc(fn, builtinsregistry.GetBuiltinProperties) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scbuild/BUILD.bazel b/pkg/sql/schemachanger/scbuild/BUILD.bazel index 9def006efc1f..dc87e7daad9d 100644 --- a/pkg/sql/schemachanger/scbuild/BUILD.bazel +++ b/pkg/sql/schemachanger/scbuild/BUILD.bazel @@ -37,7 +37,7 @@ go_library( "//pkg/sql/schemachanger/scerrors", "//pkg/sql/schemachanger/scpb", "//pkg/sql/schemachanger/screl", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", "//pkg/sql/sem/eval", diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index fa08813863ee..00a9dc58bbf7 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -33,7 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -411,7 +411,7 @@ func (b *builderState) WrapExpression(parentID catid.DescID, expr tree.Expr) *sc // Collect sequence IDs. var seqIDs catalog.DescriptorIDSet { - seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtinsregistry.GetBuiltinProperties) if err != nil { panic(err) } @@ -434,7 +434,7 @@ func (b *builderState) WrapExpression(parentID catid.DescID, expr tree.Expr) *sc seqIDs.Add(seq.SequenceID) } if len(seqNameToID) > 0 { - expr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtins.GetBuiltinProperties) + expr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtinsregistry.GetBuiltinProperties) if err != nil { panic(err) } diff --git a/pkg/sql/schemachanger/scdecomp/BUILD.bazel b/pkg/sql/schemachanger/scdecomp/BUILD.bazel index bb1cb2051b1c..0c93864f6ee8 100644 --- a/pkg/sql/schemachanger/scdecomp/BUILD.bazel +++ b/pkg/sql/schemachanger/scdecomp/BUILD.bazel @@ -18,7 +18,7 @@ go_library( "//pkg/sql/parser", "//pkg/sql/schemachanger/scerrors", "//pkg/sql/schemachanger/scpb", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", "//pkg/sql/sem/tree", diff --git a/pkg/sql/schemachanger/scdecomp/helpers.go b/pkg/sql/schemachanger/scdecomp/helpers.go index 730db8d1d59a..18584946ace8 100644 --- a/pkg/sql/schemachanger/scdecomp/helpers.go +++ b/pkg/sql/schemachanger/scdecomp/helpers.go @@ -19,7 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/lib/pq/oid" @@ -67,7 +67,7 @@ func (w *walkCtx) newExpression(expr string) (*scpb.Expression, error) { } var seqIDs catalog.DescriptorIDSet { - seqIdents, err := seqexpr.GetUsedSequences(e, builtins.GetBuiltinProperties) + seqIdents, err := seqexpr.GetUsedSequences(e, builtinsregistry.GetBuiltinProperties) if err != nil { return nil, err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/BUILD.bazel b/pkg/sql/schemachanger/scexec/scmutationexec/BUILD.bazel index 2cfbf883004c..17e4d958cc1e 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/BUILD.bazel +++ b/pkg/sql/schemachanger/scexec/scmutationexec/BUILD.bazel @@ -34,7 +34,7 @@ go_library( "//pkg/sql/schemachanger/scop", "//pkg/sql/schemachanger/scpb", "//pkg/sql/schemachanger/screl", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/catid", "//pkg/sql/sem/tree", "//pkg/sql/types", diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/helpers.go b/pkg/sql/schemachanger/scexec/scmutationexec/helpers.go index c52c4212146c..84a751f0aacc 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/helpers.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/helpers.go @@ -22,7 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" ) @@ -255,7 +255,7 @@ func sequenceIDsInExpr(expr string) (ids catalog.DescriptorIDSet, _ error) { if err != nil { return ids, err } - seqIdents, err := seqexpr.GetUsedSequences(e, builtins.GetBuiltinProperties) + seqIdents, err := seqexpr.GetUsedSequences(e, builtinsregistry.GetBuiltinProperties) if err != nil { return ids, err } diff --git a/pkg/sql/sem/builtins/BUILD.bazel b/pkg/sql/sem/builtins/BUILD.bazel index b30ad02a6150..7a6264a269fc 100644 --- a/pkg/sql/sem/builtins/BUILD.bazel +++ b/pkg/sql/sem/builtins/BUILD.bazel @@ -69,6 +69,7 @@ go_library( "//pkg/sql/rowenc/valueside", "//pkg/sql/sem/asof", "//pkg/sql/sem/builtins/builtinconstants", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", "//pkg/sql/sem/eval", @@ -154,6 +155,7 @@ go_test( "//pkg/sql/pgwire/pgerror", "//pkg/sql/randgen", "//pkg/sql/sem/builtins/builtinconstants", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/sem/tree/treewindow", diff --git a/pkg/sql/sem/builtins/aggregate_builtins.go b/pkg/sql/sem/builtins/aggregate_builtins.go index 30e3a0e49577..634c08386001 100644 --- a/pkg/sql/sem/builtins/aggregate_builtins.go +++ b/pkg/sql/sem/builtins/aggregate_builtins.go @@ -40,9 +40,6 @@ import ( func initAggregateBuiltins() { // Add all aggregates to the builtins map after a few sanity checks. for k, v := range aggregates { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } if v.props.Class != tree.AggregateClass { panic(errors.AssertionFailedf("%s: aggregate functions should be marked with the tree.AggregateClass "+ @@ -59,7 +56,7 @@ func initAggregateBuiltins() { } } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/all_builtins.go b/pkg/sql/sem/builtins/all_builtins.go index f5ce98fffa43..681b76f1eaa0 100644 --- a/pkg/sql/sem/builtins/all_builtins.go +++ b/pkg/sql/sem/builtins/all_builtins.go @@ -15,6 +15,7 @@ import ( "sort" "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -34,6 +35,7 @@ var AllAggregateBuiltinNames []string var AllWindowBuiltinNames []string func init() { + initRegularBuiltins() initAggregateBuiltins() initWindowBuiltins() initGeneratorBuiltins() @@ -46,68 +48,64 @@ func init() { initPgcryptoBuiltins() initProbeRangesBuiltins() - AllBuiltinNames = make([]string, 0, len(builtins)) - AllAggregateBuiltinNames = make([]string, 0, len(aggregates)) tree.FunDefs = make(map[string]*tree.FunctionDefinition) - for name, def := range builtins { - fDef := tree.NewFunctionDefinition(name, &def.props, def.overloads) + builtinsregistry.Iterate(func(name string, props *tree.FunctionProperties, overloads []tree.Overload) { + fDef := tree.NewFunctionDefinition(name, props, overloads) tree.FunDefs[name] = fDef if !fDef.ShouldDocument() { // Avoid listing help for undocumented functions. - continue + return } AllBuiltinNames = append(AllBuiltinNames, name) - if def.props.Class == tree.AggregateClass { + if props.Class == tree.AggregateClass { AllAggregateBuiltinNames = append(AllAggregateBuiltinNames, name) - } else if def.props.Class == tree.WindowClass { + } else if props.Class == tree.WindowClass { AllWindowBuiltinNames = append(AllWindowBuiltinNames, name) } - for i, overload := range def.overloads { - fnCount := 0 - if overload.Fn != nil { - fnCount++ - } - if overload.FnWithExprs != nil { - fnCount++ - } - if overload.Generator != nil { - overload.Fn = unsuitableUseOfGeneratorFn - overload.FnWithExprs = unsuitableUseOfGeneratorFnWithExprs - fnCount++ - } - if overload.GeneratorWithExprs != nil { - overload.Fn = unsuitableUseOfGeneratorFn - overload.FnWithExprs = unsuitableUseOfGeneratorFnWithExprs - fnCount++ - } - if fnCount > 1 { - panic(fmt.Sprintf( - "builtin %s: at most 1 of Fn, FnWithExprs, Generator, and GeneratorWithExprs"+ - "must be set on overloads; (found %d)", - name, fnCount, - )) - } - c := sqltelemetry.BuiltinCounter(name, overload.Signature(false)) - def.overloads[i].OnTypeCheck = func() { - telemetry.Inc(c) - } - } - } - - // Generate missing categories. - for _, name := range AllBuiltinNames { - def := builtins[name] - if def.props.Category == "" { - def.props.Category = getCategory(def.overloads) - builtins[name] = def - } - } + }) sort.Strings(AllBuiltinNames) sort.Strings(AllAggregateBuiltinNames) sort.Strings(AllWindowBuiltinNames) } +func registerBuiltin(name string, def builtinDefinition) { + for i, overload := range def.overloads { + fnCount := 0 + if overload.Fn != nil { + fnCount++ + } + if overload.FnWithExprs != nil { + fnCount++ + } + if overload.Generator != nil { + overload.Fn = unsuitableUseOfGeneratorFn + overload.FnWithExprs = unsuitableUseOfGeneratorFnWithExprs + fnCount++ + } + if overload.GeneratorWithExprs != nil { + overload.Fn = unsuitableUseOfGeneratorFn + overload.FnWithExprs = unsuitableUseOfGeneratorFnWithExprs + fnCount++ + } + if fnCount > 1 { + panic(fmt.Sprintf( + "builtin %s: at most 1 of Fn, FnWithExprs, Generator, and GeneratorWithExprs"+ + "must be set on overloads; (found %d)", + name, fnCount, + )) + } + c := sqltelemetry.BuiltinCounter(name, overload.Signature(false)) + def.overloads[i].OnTypeCheck = func() { + telemetry.Inc(c) + } + } + if def.props.ShouldDocument() && def.props.Category == "" { + def.props.Category = getCategory(def.overloads) + } + builtinsregistry.Register(name, &def.props, def.overloads) +} + func getCategory(b []tree.Overload) string { // If single argument attempt to categorize by the type of the argument. for _, ovl := range b { diff --git a/pkg/sql/sem/builtins/all_builtins_test.go b/pkg/sql/sem/builtins/all_builtins_test.go index e10a41b66c00..2abbea9ffe5a 100644 --- a/pkg/sql/sem/builtins/all_builtins_test.go +++ b/pkg/sql/sem/builtins/all_builtins_test.go @@ -17,6 +17,7 @@ import ( "strings" "testing" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -28,8 +29,8 @@ import ( func TestOverloadsHaveVolatility(t *testing.T) { defer leaktest.AfterTest(t)() - for name, builtin := range builtins { - for idx, overload := range builtin.overloads { + builtinsregistry.Iterate(func(name string, props *tree.FunctionProperties, overloads []tree.Overload) { + for idx, overload := range overloads { assert.NotEqual( t, volatility.V(0), @@ -39,7 +40,7 @@ func TestOverloadsHaveVolatility(t *testing.T) { idx, ) } - } + }) } // TestOverloadsVolatilityMatchesPostgres that our overloads match Postgres' @@ -140,8 +141,8 @@ func TestOverloadsVolatilityMatchesPostgres(t *testing.T) { } // Check each builtin against Postgres. - for name, builtin := range builtins { - for idx, overload := range builtin.overloads { + builtinsregistry.Iterate(func(name string, props *tree.FunctionProperties, overloads []tree.Overload) { + for idx, overload := range overloads { if overload.IgnoreVolatilityCheck { continue } @@ -160,5 +161,5 @@ func TestOverloadsVolatilityMatchesPostgres(t *testing.T) { postgresVolatility, ) } - } + }) } diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 8415ba9f382b..89667c34825c 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -148,17 +148,6 @@ type builtinDefinition struct { overloads []tree.Overload } -// GetBuiltinProperties provides low-level access to a built-in function's properties. -// For a better, semantic-rich interface consider using tree.FunctionDefinition -// instead, and resolve function names via ResolvableFunctionReference.Resolve(). -func GetBuiltinProperties(name string) (*tree.FunctionProperties, []tree.Overload) { - def, ok := builtins[name] - if !ok { - return nil, nil - } - return &def.props, def.overloads -} - // defProps is used below to define built-in functions with default properties. func defProps() tree.FunctionProperties { return tree.FunctionProperties{} } @@ -192,10 +181,16 @@ func mustBeDIntInTenantRange(e tree.Expr) (tree.DInt, error) { return tenID, nil } +func initRegularBuiltins() { + for k, v := range regularBuiltins { + registerBuiltin(k, v) + } +} + // builtins contains the built-in functions indexed by name. // // For use in other packages, see AllBuiltinNames and GetBuiltinProperties(). -var builtins = map[string]builtinDefinition{ +var regularBuiltins = map[string]builtinDefinition{ // TODO(XisiHuang): support encoding, i.e., length(str, encoding). "length": lengthImpls(true /* includeBitOverload */), "char_length": lengthImpls(false /* includeBitOverload */), diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index bf39fa22865c..b80dd55fe3aa 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -38,16 +39,20 @@ import ( func TestCategory(t *testing.T) { defer leaktest.AfterTest(t)() - if expected, actual := builtinconstants.CategoryString, builtins["lower"].props.Category; expected != actual { + getCategory := func(name string) string { + props, _ := builtinsregistry.GetBuiltinProperties(name) + return props.Category + } + if expected, actual := builtinconstants.CategoryString, getCategory("lower"); expected != actual { t.Fatalf("bad category: expected %q got %q", expected, actual) } - if expected, actual := builtinconstants.CategoryString, builtins["length"].props.Category; expected != actual { + if expected, actual := builtinconstants.CategoryString, getCategory("length"); expected != actual { t.Fatalf("bad category: expected %q got %q", expected, actual) } - if expected, actual := builtinconstants.CategoryDateAndTime, builtins["now"].props.Category; expected != actual { + if expected, actual := builtinconstants.CategoryDateAndTime, getCategory("now"); expected != actual { t.Fatalf("bad category: expected %q got %q", expected, actual) } - if expected, actual := builtinconstants.CategorySystemInfo, builtins["version"].props.Category; expected != actual { + if expected, actual := builtinconstants.CategorySystemInfo, getCategory("version"); expected != actual { t.Fatalf("bad category: expected %q got %q", expected, actual) } } diff --git a/pkg/sql/sem/builtins/builtinsregistry/BUILD.bazel b/pkg/sql/sem/builtins/builtinsregistry/BUILD.bazel new file mode 100644 index 000000000000..0d0fd6315295 --- /dev/null +++ b/pkg/sql/sem/builtins/builtinsregistry/BUILD.bazel @@ -0,0 +1,9 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "builtinsregistry", + srcs = ["builtins_registry.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry", + visibility = ["//visibility:public"], + deps = ["//pkg/sql/sem/tree"], +) diff --git a/pkg/sql/sem/builtins/builtinsregistry/builtins_registry.go b/pkg/sql/sem/builtins/builtinsregistry/builtins_registry.go new file mode 100644 index 000000000000..5eef0bd1b6e6 --- /dev/null +++ b/pkg/sql/sem/builtins/builtinsregistry/builtins_registry.go @@ -0,0 +1,56 @@ +// Copyright 2022 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. + +// Package builtinsregistry stores the definitions of builtin functions. +// +// The builtins package imports this package and registers its functions. +// This package exists to avoid import cycles so that catalog packages +// can interact with builtin definitions without needing to import the +// builtins package. +package builtinsregistry + +import "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + +var registry = map[string]definition{} + +// Register registers a builtin. Intending to be called at init time, it panics +// if a function of the same name has already been registered. +func Register(name string, props *tree.FunctionProperties, overloads []tree.Overload) { + if _, exists := registry[name]; exists { + panic("duplicate builtin: " + name) + } + registry[name] = definition{ + props: props, + overloads: overloads, + } +} + +// GetBuiltinProperties provides low-level access to a built-in function's properties. +// For a better, semantic-rich interface consider using tree.FunctionDefinition +// instead, and resolve function names via ResolvableFunctionReference.Resolve(). +func GetBuiltinProperties(name string) (*tree.FunctionProperties, []tree.Overload) { + def, ok := registry[name] + if !ok { + return nil, nil + } + return def.props, def.overloads +} + +// Iterate iterates the previously registered functions. +func Iterate(f func(name string, props *tree.FunctionProperties, overloads []tree.Overload)) { + for k, v := range registry { + f(k, v.props, v.overloads) + } +} + +type definition struct { + props *tree.FunctionProperties + overloads []tree.Overload +} diff --git a/pkg/sql/sem/builtins/generator_builtins.go b/pkg/sql/sem/builtins/generator_builtins.go index 4b79c9485108..b749ba03c61a 100644 --- a/pkg/sql/sem/builtins/generator_builtins.go +++ b/pkg/sql/sem/builtins/generator_builtins.go @@ -49,16 +49,11 @@ var _ eval.ValueGenerator = &arrayValueGenerator{} func initGeneratorBuiltins() { // Add all windows to the builtins map after a few sanity checks. for k, v := range generators { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - if v.props.Class != tree.GeneratorClass { panic(errors.AssertionFailedf("generator functions should be marked with the tree.GeneratorClass "+ "function class, found %v", v)) } - - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/generator_probe_ranges.go b/pkg/sql/sem/builtins/generator_probe_ranges.go index b55affd8b5dc..fa5ac7102ed4 100644 --- a/pkg/sql/sem/builtins/generator_probe_ranges.go +++ b/pkg/sql/sem/builtins/generator_probe_ranges.go @@ -36,16 +36,11 @@ import ( func initProbeRangesBuiltins() { // Add all windows to the Builtins map after a few sanity checks. for k, v := range probeRangesGenerators { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - if v.props.Class != tree.GeneratorClass { panic(errors.AssertionFailedf("generator functions should be marked with the tree.GeneratorClass "+ "function class, found %v", v)) } - - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/geo_builtins.go b/pkg/sql/sem/builtins/geo_builtins.go index 9308d4e76531..465c5e1d6a20 100644 --- a/pkg/sql/sem/builtins/geo_builtins.go +++ b/pkg/sql/sem/builtins/geo_builtins.go @@ -7306,12 +7306,9 @@ func initGeoBuiltins() { } for k, v := range geoBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } v.props.Category = builtinconstants.CategorySpatial v.props.AvailableOnPublicSchema = true - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/help_test.go b/pkg/sql/sem/builtins/help_test.go index 8f782e1bbe91..bb71447a3a0f 100644 --- a/pkg/sql/sem/builtins/help_test.go +++ b/pkg/sql/sem/builtins/help_test.go @@ -18,15 +18,17 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/util/leaktest" ) func TestHelpFunctions(t *testing.T) { defer leaktest.AfterTest(t)() // This test checks that all the built-in functions receive contextual help. - for f := range builtins { + builtinsregistry.Iterate(func(f string, _ *tree.FunctionProperties, _ []tree.Overload) { if unicode.IsUpper(rune(f[0])) { - continue + return } t.Run(f, func(t *testing.T) { _, err := parser.Parse("select " + f + "(??") @@ -52,5 +54,5 @@ func TestHelpFunctions(t *testing.T) { t.Errorf("help text didn't match %q:\n%s", pattern, help) } }) - } + }) } diff --git a/pkg/sql/sem/builtins/math_builtins.go b/pkg/sql/sem/builtins/math_builtins.go index 902a710e44cc..afa2540fcf1d 100644 --- a/pkg/sql/sem/builtins/math_builtins.go +++ b/pkg/sql/sem/builtins/math_builtins.go @@ -28,10 +28,7 @@ import ( func initMathBuiltins() { // Add all mathBuiltins to the builtins map after a sanity check. for k, v := range mathBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/overlaps_builtins.go b/pkg/sql/sem/builtins/overlaps_builtins.go index b85666945b2e..44fb61ad68ef 100644 --- a/pkg/sql/sem/builtins/overlaps_builtins.go +++ b/pkg/sql/sem/builtins/overlaps_builtins.go @@ -48,10 +48,7 @@ var ( func initOverlapsBuiltins() { // Add all overlapsBuiltins to the builtins map after a sanity check. for k, v := range overlapsBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index bece1957282e..8bbca082120b 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -99,11 +99,8 @@ func PGIOBuiltinPrefix(typ *types.T) string { // initPGBuiltins adds all of the postgres builtins to the builtins map. func initPGBuiltins() { for k, v := range pgBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } v.props.Category = builtinconstants.CategoryCompatibility - builtins[k] = v + registerBuiltin(k, v) } // Make non-array type i/o builtins. @@ -118,19 +115,19 @@ func initPGBuiltins() { } builtinPrefix := PGIOBuiltinPrefix(typ) for name, builtin := range makeTypeIOBuiltins(builtinPrefix, typ) { - builtins[name] = builtin + registerBuiltin(name, builtin) } } // Make array type i/o builtins. for name, builtin := range makeTypeIOBuiltins("array_", types.AnyArray) { - builtins[name] = builtin + registerBuiltin(name, builtin) } for name, builtin := range makeTypeIOBuiltins("anyarray_", types.AnyArray) { - builtins[name] = builtin + registerBuiltin(name, builtin) } // Make enum type i/o builtins. for name, builtin := range makeTypeIOBuiltins("enum_", types.AnyEnum) { - builtins[name] = builtin + registerBuiltin(name, builtin) } // Make crdb_internal.create_regfoo and to_regfoo builtins. @@ -146,8 +143,8 @@ func initPGBuiltins() { {"Translates a textual type name to its OID", types.RegType}, } { typName := b.typ.SQLStandardName() - builtins["crdb_internal.create_"+typName] = makeCreateRegDef(b.typ) - builtins["to_"+typName] = makeToRegOverload(b.typ, b.toRegOverloadHelpText) + registerBuiltin("crdb_internal.create_"+typName, makeCreateRegDef(b.typ)) + registerBuiltin("to_"+typName, makeToRegOverload(b.typ, b.toRegOverloadHelpText)) } } diff --git a/pkg/sql/sem/builtins/pgcrypto_builtins.go b/pkg/sql/sem/builtins/pgcrypto_builtins.go index 702ed58996bc..9cd60d2877e2 100644 --- a/pkg/sql/sem/builtins/pgcrypto_builtins.go +++ b/pkg/sql/sem/builtins/pgcrypto_builtins.go @@ -37,10 +37,7 @@ import ( func initPgcryptoBuiltins() { // Add all pgcryptoBuiltins to the builtins map after a sanity check. for k, v := range pgcryptoBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/replication_builtins.go b/pkg/sql/sem/builtins/replication_builtins.go index 2001bd14936b..1ebda888e472 100644 --- a/pkg/sql/sem/builtins/replication_builtins.go +++ b/pkg/sql/sem/builtins/replication_builtins.go @@ -28,10 +28,7 @@ import ( func initReplicationBuiltins() { // Add all replicationBuiltins to the builtins map after a sanity check. for k, v := range replicationBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/trigram_builtins.go b/pkg/sql/sem/builtins/trigram_builtins.go index 61fd541bdbdc..95b1ee3a808d 100644 --- a/pkg/sql/sem/builtins/trigram_builtins.go +++ b/pkg/sql/sem/builtins/trigram_builtins.go @@ -21,12 +21,9 @@ import ( func initTrigramBuiltins() { for k, v := range trigramBuiltins { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } v.props.Category = builtinconstants.CategoryTrigram v.props.AvailableOnPublicSchema = true - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/builtins/window_builtins.go b/pkg/sql/sem/builtins/window_builtins.go index b6ca330cb423..3a4f6082f255 100644 --- a/pkg/sql/sem/builtins/window_builtins.go +++ b/pkg/sql/sem/builtins/window_builtins.go @@ -25,10 +25,6 @@ import ( func initWindowBuiltins() { // Add all windows to the builtins map after a few sanity checks. for k, v := range windows { - if _, exists := builtins[k]; exists { - panic("duplicate builtin: " + k) - } - if v.props.Class != tree.WindowClass { panic(errors.AssertionFailedf("%s: window functions should be marked with the tree.WindowClass "+ "function class, found %v", k, v)) @@ -39,7 +35,7 @@ func initWindowBuiltins() { "found %v", k, w)) } } - builtins[k] = v + registerBuiltin(k, v) } } diff --git a/pkg/sql/sem/tree/function_definition.go b/pkg/sql/sem/tree/function_definition.go index d47594407331..cb83d94a8ced 100644 --- a/pkg/sql/sem/tree/function_definition.go +++ b/pkg/sql/sem/tree/function_definition.go @@ -155,6 +155,10 @@ func NewFunctionDefinition( // FunDefs holds pre-allocated FunctionDefinition instances // for every builtin function. Initialized by builtins.init(). +// +// Note that this is extremely similar to the set stored in builtinsregistry. +// The hope is to remove this map at some point in the future as we delegate +// function definition resolution to interfaces defined in the SemaContext. var FunDefs map[string]*FunctionDefinition // OidToBuiltinName contains a map from the hashed OID of all builtin functions diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 30db52e91c5e..53bc836a2c27 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -30,6 +30,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -826,7 +827,7 @@ func maybeAddSequenceDependencies( backrefs map[descpb.ID]*tabledesc.Mutable, colExprKind tabledesc.ColExprKind, ) ([]*tabledesc.Mutable, error) { - seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtinsregistry.GetBuiltinProperties) if err != nil { return nil, err } @@ -901,7 +902,7 @@ func maybeAddSequenceDependencies( // If sequences are present in the expr (and the cluster is the right version), // walk the expr tree and replace any sequences names with their IDs. if len(seqIdentifiers) > 0 { - newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtins.GetBuiltinProperties) + newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtinsregistry.GetBuiltinProperties) if err != nil { return nil, err } diff --git a/pkg/sql/tests/BUILD.bazel b/pkg/sql/tests/BUILD.bazel index 0148051f2559..c3105527b845 100644 --- a/pkg/sql/tests/BUILD.bazel +++ b/pkg/sql/tests/BUILD.bazel @@ -84,6 +84,7 @@ go_test( "//pkg/sql/pgwire/pgcode", "//pkg/sql/randgen", "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/sessiondatapb", diff --git a/pkg/sql/tests/rsg_test.go b/pkg/sql/tests/rsg_test.go index 32d5bfd2bf61..c66ad6068172 100644 --- a/pkg/sql/tests/rsg_test.go +++ b/pkg/sql/tests/rsg_test.go @@ -32,6 +32,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/tests" @@ -303,7 +304,7 @@ func TestRandomSyntaxFunctions(t *testing.T) { // Skipped due to long execution time. continue } - _, variations := builtins.GetBuiltinProperties(name) + _, variations := builtinsregistry.GetBuiltinProperties(name) for _, builtin := range variations { select { case <-done: diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index bd4a6ef4d6a8..6ec8e6d32dd5 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -50,7 +50,7 @@ go_library( "//pkg/sql/catalog/typedesc", "//pkg/sql/parser", "//pkg/sql/privilege", - "//pkg/sql/sem/builtins", + "//pkg/sql/sem/builtins/builtinsregistry", "//pkg/sql/sem/tree", "//pkg/sql/sessiondata", "//pkg/sql/sqlutil", diff --git a/pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go b/pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go index 462c3d716334..6bfbed0a8d1c 100644 --- a/pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go +++ b/pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go @@ -23,7 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/parser" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/upgrade" @@ -176,7 +176,7 @@ func upgradeSequenceReferenceInTable( if err != nil { return err } - seqIdentifiers, err := seqexpr.GetUsedSequences(parsedExpr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(parsedExpr, builtinsregistry.GetBuiltinProperties) if err != nil { return err } @@ -190,7 +190,7 @@ func upgradeSequenceReferenceInTable( } // Perform the sequence replacement in the default expression. - newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(parsedExpr, seqNameToID, builtins.GetBuiltinProperties) + newExpr, err := seqexpr.ReplaceSequenceNamesWithIDs(parsedExpr, seqNameToID, builtinsregistry.GetBuiltinProperties) if err != nil { return err } @@ -235,7 +235,7 @@ func upgradeSequenceReferenceInView( ) error { var changedSeqDescs []*tabledesc.Mutable replaceSeqFunc := func(expr tree.Expr) (recurse bool, newExpr tree.Expr, err error) { - seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtins.GetBuiltinProperties) + seqIdentifiers, err := seqexpr.GetUsedSequences(expr, builtinsregistry.GetBuiltinProperties) if err != nil { return false, expr, err } @@ -244,7 +244,7 @@ func upgradeSequenceReferenceInView( return false, expr, err } - newExpr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtins.GetBuiltinProperties) + newExpr, err = seqexpr.ReplaceSequenceNamesWithIDs(expr, seqNameToID, builtinsregistry.GetBuiltinProperties) if err != nil { return false, expr, err } From 177816c4c7a2eded1b71420b0a578e8b1d2701e7 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Wed, 13 Jul 2022 14:34:47 -0500 Subject: [PATCH 2/3] opt: add assertion that selectivity is never NaN This commit addresses a leftover comment from #84366. Release note: None --- pkg/sql/opt/props/selectivity.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/sql/opt/props/selectivity.go b/pkg/sql/opt/props/selectivity.go index f4e0415250e9..000942d14384 100644 --- a/pkg/sql/opt/props/selectivity.go +++ b/pkg/sql/opt/props/selectivity.go @@ -10,7 +10,12 @@ package props -import "fmt" +import ( + "fmt" + "math" + + "github.com/cockroachdb/errors" +) // epsilon is the minimum value Selectivity can hold, since it cannot be 0. const epsilon = 1e-10 @@ -100,6 +105,9 @@ func MaxSelectivity(a, b Selectivity) Selectivity { // selectivityInRange performs the range check, if the selectivity falls // outside of the range, this method will return the appropriate min/max value. func selectivityInRange(sel float64) float64 { + if math.IsNaN(sel) { + panic(errors.AssertionFailedf("selectivity is NaN")) + } switch { case sel < epsilon: return epsilon From 2b6263195359995ff072b4eb9e0040df5abfd782 Mon Sep 17 00:00:00 2001 From: Lidor Carmel Date: Wed, 13 Jul 2022 14:33:49 -0700 Subject: [PATCH 3/3] roachtest: wait for a 5x replication instead of 3x Flaky test: we wait for a 3x replication and then drain 3 nodes. Then we sometimes have ranges with all 3 replicas on those 3 nodes, stuck forever. Instead the test should wait for a 5x replication before starting the drain. Fixes #84128. Release note: None --- pkg/cmd/roachtest/tests/decommission.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/tests/decommission.go b/pkg/cmd/roachtest/tests/decommission.go index e801b583a106..f7a2de59e896 100644 --- a/pkg/cmd/roachtest/tests/decommission.go +++ b/pkg/cmd/roachtest/tests/decommission.go @@ -160,7 +160,7 @@ func runDrainAndDecommission( run(`SET CLUSTER SETTING kv.snapshot_recovery.max_rate='2GiB'`) // Wait for initial up-replication. - err := WaitFor3XReplication(ctx, t, db) + err := WaitForReplication(ctx, t, db, defaultReplicationFactor) require.NoError(t, err) }