Skip to content

Commit

Permalink
sql/storageparam: break builtins dep on tabledesc
Browse files Browse the repository at this point in the history 
The TableStorageParamObserver meant that paramparse and transitively builtins
depended on tabledesc. This was unfortunate because we want seqexpr is depended
on by builtins and we want to use seqexpr in tabledesc, so we have to make
sure that builtins does not depend on tabledesc.

This commit achieves that goal by splitting out paramparse into three new
packages from paramparse:
 * sql/storageparam: defines the interface for the Setter, contains functions
   to drive forward the setting and resetting of params, and has some shared
   functionality.
 * sql/storageparam/indexstorageparam: implementation of storageparam.Setter
   for the `descpb.IndexDescriptor`.
 * sql/storageparam/tablestorageparam: implementation of storageparam.Setter
   for the `*tabledesc.Mutable`.

This allows the `builtins` package to use the `indexstorageparam` package
cleanly without depending on `*tabledesc.Mutable`. It also recognizes that
lots of utility methods in `paramparse` aren't about `storageparam`s.

Release note: None
  • Loading branch information
@ajwerner
ajwerner committed Jun 7, 2022
1 parent e280a7f commit ab5b9bb
Show file tree
Hide file tree
Showing 14 changed files with 464 additions and 349 deletions.
3 changes: 3 additions & 0 deletions pkg/sql/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,9 @@ go_library(
"//pkg/sql/sqlutil",
"//pkg/sql/stats",
"//pkg/sql/stmtdiagnostics",
"//pkg/sql/storageparam",
"//pkg/sql/storageparam/indexstorageparam",
"//pkg/sql/storageparam/tablestorageparam",
"//pkg/sql/types",
"//pkg/sql/vtable",
"//pkg/storage/enginepb",
Expand Down
11 changes: 6 additions & 5 deletions pkg/sql/alter_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
Expand All @@ -43,6 +42,8 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/sql/stats"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
Expand Down Expand Up @@ -727,12 +728,12 @@ func (n *alterTableNode) startExec(params runParams) error {
if ttl := n.tableDesc.GetRowLevelTTL(); ttl != nil {
ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
}
if err := paramparse.SetStorageParameters(
if err := storageparam.Set(
params.ctx,
params.p.SemaCtx(),
params.EvalContext(),
t.StorageParams,
paramparse.NewTableStorageParamObserver(n.tableDesc),
tablestorageparam.NewSetter(n.tableDesc),
); err != nil {
return err
}
Expand Down Expand Up @@ -761,11 +762,11 @@ func (n *alterTableNode) startExec(params runParams) error {
if ttl := n.tableDesc.GetRowLevelTTL(); ttl != nil {
ttlBefore = protoutil.Clone(ttl).(*catpb.RowLevelTTL)
}
if err := paramparse.ResetStorageParameters(
if err := storageparam.Reset(
params.ctx,
params.EvalContext(),
t.Params,
paramparse.NewTableStorageParamObserver(n.tableDesc),
tablestorageparam.NewSetter(n.tableDesc),
); err != nil {
return err
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/sql/create_index.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
Expand All @@ -32,6 +31,8 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -267,12 +268,12 @@ func makeIndexDescriptor(
return nil, err
}

if err := paramparse.SetStorageParameters(
if err := storageparam.Set(
params.ctx,
params.p.SemaCtx(),
params.EvalContext(),
n.StorageParams,
&paramparse.IndexStorageParamObserver{IndexDesc: &indexDesc},
&indexstorageparam.Setter{IndexDesc: &indexDesc},
); err != nil {
return nil, err
}
Expand Down
11 changes: 7 additions & 4 deletions pkg/sql/create_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
Expand Down Expand Up @@ -1313,12 +1316,12 @@ func NewTableDesc(
id, dbID, sc.GetID(), n.Table.Table(), creationTime, privileges, persistence,
)

if err := paramparse.SetStorageParameters(
if err := storageparam.Set(
ctx,
semaCtx,
evalCtx,
n.StorageParams,
paramparse.NewTableStorageParamObserver(&desc),
tablestorageparam.NewSetter(&desc),
); err != nil {
return nil, err
}
Expand Down Expand Up @@ -1854,12 +1857,12 @@ func NewTableDesc(
}
idx.Predicate = expr
}
if err := paramparse.SetStorageParameters(
if err := storageparam.Set(
ctx,
semaCtx,
evalCtx,
d.StorageParams,
&paramparse.IndexStorageParamObserver{IndexDesc: &idx},
&indexstorageparam.Setter{IndexDesc: &idx},
); err != nil {
return nil, err
}
Expand Down
12 changes: 0 additions & 12 deletions pkg/sql/paramparse/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,17 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
go_library(
name = "paramparse",
srcs = [
"paramobserver.go",
"paramparse.go",
"validation.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/paramparse",
visibility = ["//visibility:public"],
deps = [
"//pkg/geo/geoindex",
"//pkg/server/telemetry",
"//pkg/settings",
"//pkg/sql/catalog/catpb",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/tabledesc",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/pgwire/pgnotice",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/normalize",
"//pkg/sql/sem/tree",
"//pkg/sql/sqltelemetry",
"//pkg/sql/types",
"//pkg/util/duration",
"//pkg/util/errorutil/unimplemented",
"@com_github_cockroachdb_errors//:errors",
],
)
3 changes: 1 addition & 2 deletions pkg/sql/paramparse/paramparse.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

// Package paramparse parses parameters that are set in param lists
// or session vars.
// Package paramparse provides utilities for parsing storage paramaters.
package paramparse

import (
Expand Down
7 changes: 6 additions & 1 deletion pkg/sql/sem/builtins/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ go_library(
"//pkg/sql/lex",
"//pkg/sql/lexbase",
"//pkg/sql/memsize",
"//pkg/sql/paramparse",
"//pkg/sql/parser",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
Expand All @@ -78,6 +77,8 @@ go_library(
"//pkg/sql/sqlliveness",
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil",
"//pkg/sql/sqltelemetry",
"//pkg/sql/storageparam",
"//pkg/sql/storageparam/indexstorageparam",
"//pkg/sql/types",
"//pkg/streaming",
"//pkg/util",
Expand Down Expand Up @@ -174,5 +175,9 @@ disallowed_imports_test(
src = "builtins",
disallowed_list = [
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/tabledesc",
"//pkg/sql/catalog/schemadesc",
"//pkg/sql/catalog/dbdesc",
"//pkg/sql/catalog/typedesc",
],
)
7 changes: 4 additions & 3 deletions pkg/sql/sem/builtins/geo_builtins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,14 @@ import (
"github.com/cockroachdb/cockroach/pkg/geo/twkb"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
Expand Down Expand Up @@ -7671,12 +7672,12 @@ func applyGeoindexConfigStorageParams(
return geoindex.Config{}, errors.Newf("invalid storage parameters specified: %s", params)
}
semaCtx := tree.MakeSemaContext()
if err := paramparse.SetStorageParameters(
if err := storageparam.Set(
evalCtx.Context,
&semaCtx,
evalCtx,
stmt.AST.(*tree.CreateIndex).StorageParams,
&paramparse.IndexStorageParamObserver{IndexDesc: indexDesc},
&indexstorageparam.Setter{IndexDesc: indexDesc},
); err != nil {
return geoindex.Config{}, err
}
Expand Down
20 changes: 20 additions & 0 deletions pkg/sql/storageparam/BUILD.bazel
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "storageparam",
srcs = ["storage_param.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/storageparam",
visibility = ["//visibility:public"],
deps = [
"//pkg/server/telemetry",
"//pkg/sql/paramparse",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/pgwire/pgnotice",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/normalize",
"//pkg/sql/sem/tree",
"//pkg/sql/sqltelemetry",
"//pkg/sql/types",
],
)
20 changes: 20 additions & 0 deletions pkg/sql/storageparam/indexstorageparam/BUILD.bazel
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "indexstorageparam",
srcs = ["index_storage_param.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam",
visibility = ["//visibility:public"],
deps = [
"//pkg/geo/geoindex",
"//pkg/sql/catalog/descpb",
"//pkg/sql/paramparse",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/storageparam",
"//pkg/util/errorutil/unimplemented",
"@com_github_cockroachdb_errors//:errors",
],
)
Loading

0 comments on commit ab5b9bb

Please sign in to comment.