Skip to content

Commit

Permalink
opt: break memo's dependence on builtins
Browse files Browse the repository at this point in the history
This commit breaks the `memo` package's dependence on the `builtins`
package so that they can be compiled concurrently. Optimizer packages
are no longer on the critical path, for now.

Informs #79357

Release note: None
  • Loading branch information
mgartner committed Apr 29, 2022
1 parent 0777ef7 commit 4287c44
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 4 deletions.
1 change: 1 addition & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ ALL_TESTS = [
"//pkg/sql/opt/invertedexpr:invertedexpr_test",
"//pkg/sql/opt/invertedidx:invertedidx_test",
"//pkg/sql/opt/lookupjoin:lookupjoin_test",
"//pkg/sql/opt/memo:memo_disallowed_imports_test",
"//pkg/sql/opt/memo:memo_test",
"//pkg/sql/opt/norm:norm_test",
"//pkg/sql/opt/opbench:opbench_test",
Expand Down
7 changes: 6 additions & 1 deletion pkg/sql/opt/memo/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")

go_library(
name = "memo",
Expand Down Expand Up @@ -35,7 +36,6 @@ go_library(
"//pkg/sql/opt/props/physical",
"//pkg/sql/rowenc/keyside",
"//pkg/sql/rowenc/valueside",
"//pkg/sql/sem/builtins",
"//pkg/sql/sem/cast",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
Expand Down Expand Up @@ -117,3 +117,8 @@ genrule(
exec_tools = ["//pkg/sql/opt/optgen/cmd/optgen"],
visibility = ["//visibility:public"],
)

disallowed_imports_test(
"memo",
["//pkg/sql/sem/builtins"],
)
10 changes: 7 additions & 3 deletions pkg/sql/opt/memo/typing.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ package memo

import (
"github.com/cockroachdb/cockroach/pkg/sql/opt"
"github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -98,11 +97,16 @@ func BinaryAllowsNullArgs(op opt.Operator, leftType, rightType *types.T) bool {
return o.NullableArgs
}

// GetBuiltinProperties is set to builtins.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)

// AggregateOverloadExists returns whether or not the given operator has a
// unary overload which takes the given type as input.
func AggregateOverloadExists(agg opt.Operator, typ *types.T) bool {
name := opt.AggregateOpReverseMap[agg]
_, overloads := builtins.GetBuiltinProperties(name)
_, overloads := GetBuiltinProperties(name)
for _, o := range overloads {
if o.Types.MatchAt(typ, 0) {
return true
Expand All @@ -117,7 +121,7 @@ func AggregateOverloadExists(agg opt.Operator, typ *types.T) bool {
func FindFunction(
e opt.ScalarExpr, name string,
) (props *tree.FunctionProperties, overload *tree.Overload, ok bool) {
props, overloads := builtins.GetBuiltinProperties(name)
props, overloads := GetBuiltinProperties(name)
for o := range overloads {
overload = &overloads[o]
if overload.Types.Length() != e.ChildCount() {
Expand Down
7 changes: 7 additions & 0 deletions pkg/sql/opt/norm/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ 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/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/types"
Expand Down Expand Up @@ -105,6 +106,12 @@ type Factory struct {
// expression that is not fully optimized.
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
}

// Init initializes a Factory structure with a new, blank memo structure inside.
// This must be called before the factory can be used (or reused).
//
Expand Down

0 comments on commit 4287c44

Please sign in to comment.