Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/schemachanger: add compatibility with 22.2 rules #95361

Merged
merged 2 commits into from
Jan 19, 2023
Merged

sql/schemachanger: add compatibility with 22.2 rules #95361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70a5c1f
sql/schemachanger: refactor rules so that they are portable between r…
fqazi Jan 12, 2023
2644ed7
sql/schemachanger: add compatibility with 22.2
fqazi Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build/bazelutil/nogo_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,8 @@
"cockroach/pkg/.*\\.eg\\.go$": "generated code",
".*\\.pb\\.go$": "generated code",
".*\\.pb\\.gw\\.go$": "generated code",
"cockroach/pkg/.*_generated\\.go$": "generated code"
"cockroach/pkg/.*_generated\\.go$": "generated code",
"cockroach/pkg/sql/schemachanger/scplan/internal/rules/.*/.*.go$": "schema changer rules"
},
"only_files": {
"cockroach/pkg/.*$": "first-party code"
Expand Down
10 changes: 8 additions & 2 deletions pkg/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,8 @@ ALL_TESTS = [
"//pkg/sql/schemachanger/scexec/backfiller:backfiller_test",
"//pkg/sql/schemachanger/scexec:scexec_test",
"//pkg/sql/schemachanger/scplan/internal/opgen:opgen_test",
"//pkg/sql/schemachanger/scplan/internal/rules:rules_test",
"//pkg/sql/schemachanger/scplan/internal/rules/current:current_test",
"//pkg/sql/schemachanger/scplan/internal/rules/release_22_2:release_22_2_test",
"//pkg/sql/schemachanger/scplan/internal/scgraph:scgraph_test",
"//pkg/sql/schemachanger/scplan:scplan_test",
"//pkg/sql/schemachanger/screl:screl_test",
Expand Down Expand Up @@ -1770,8 +1771,11 @@ GO_TARGETS = [
"//pkg/sql/schemachanger/scpb:scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen:opgen",
"//pkg/sql/schemachanger/scplan/internal/opgen:opgen_test",
"//pkg/sql/schemachanger/scplan/internal/rules/current:current",
"//pkg/sql/schemachanger/scplan/internal/rules/current:current_test",
"//pkg/sql/schemachanger/scplan/internal/rules/release_22_2:release_22_2",
"//pkg/sql/schemachanger/scplan/internal/rules/release_22_2:release_22_2_test",
"//pkg/sql/schemachanger/scplan/internal/rules:rules",
"//pkg/sql/schemachanger/scplan/internal/rules:rules_test",
"//pkg/sql/schemachanger/scplan/internal/scgraph:scgraph",
"//pkg/sql/schemachanger/scplan/internal/scgraph:scgraph_test",
"//pkg/sql/schemachanger/scplan/internal/scgraphviz:scgraphviz",
Expand Down Expand Up @@ -2919,6 +2923,8 @@ GET_X_DATA_TARGETS = [
"//pkg/sql/schemachanger/scplan:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/opgen:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/rules:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/rules/current:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/rules/release_22_2:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/scgraph:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/scgraphviz:get_x_data",
"//pkg/sql/schemachanger/scplan/internal/scstage:get_x_data",
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/schemachanger/scplan/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ go_library(
"//pkg/sql/schemachanger/scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen",
"//pkg/sql/schemachanger/scplan/internal/rules",
"//pkg/sql/schemachanger/scplan/internal/rules/current",
"//pkg/sql/schemachanger/scplan/internal/rules/release_22_2",
"//pkg/sql/schemachanger/scplan/internal/scgraph",
"//pkg/sql/schemachanger/scplan/internal/scgraphviz",
"//pkg/sql/schemachanger/scplan/internal/scstage",
Expand Down
41 changes: 2 additions & 39 deletions pkg/sql/schemachanger/scplan/internal/rules/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,25 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "rules",
srcs = [
"dep_add_column.go",
"dep_add_constraint.go",
"dep_add_index.go",
"dep_add_index_and_column.go",
"dep_add_index_and_constraint.go",
"dep_drop_column.go",
"dep_drop_constraint.go",
"dep_drop_index.go",
"dep_drop_index_and_column.go",
"dep_drop_object.go",
"dep_garbage_collection.go",
"dep_swap_index.go",
"dep_two_version.go",
"helpers.go",
"op_drop.go",
"op_index_and_column.go",
"registry.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules",
visibility = ["//pkg/sql/schemachanger/scplan:__subpackages__"],
deps = [
"//pkg/clusterversion",
"//pkg/sql/schemachanger/rel",
"//pkg/sql/schemachanger/scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen",
"//pkg/sql/schemachanger/scplan/internal/scgraph",
"//pkg/sql/schemachanger/screl",
"//pkg/sql/sem/catid",
"//pkg/util/iterutil",
"//pkg/util/log",
"//pkg/util/timeutil",
"@com_github_cockroachdb_errors//:errors",
],
)

go_test(
name = "rules_test",
srcs = [
"assertions_test.go",
"rules_test.go",
],
args = ["-test.timeout=295s"],
data = glob(["testdata/**"]),
embed = [":rules"],
deps = [
"//pkg/sql/catalog/catpb",
"//pkg/sql/schemachanger/rel",
"//pkg/sql/schemachanger/scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen",
"//pkg/sql/schemachanger/screl",
"//pkg/sql/types",
"//pkg/testutils/datapathutils",
"@com_github_cockroachdb_datadriven//:datadriven",
"@com_github_cockroachdb_errors//:errors",
"@in_gopkg_yaml_v3//:yaml_v3",
],
)
Expand Down
62 changes: 62 additions & 0 deletions pkg/sql/schemachanger/scplan/internal/rules/current/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "current",
srcs = [
"dep_add_column.go",
"dep_add_constraint.go",
"dep_add_index.go",
"dep_add_index_and_column.go",
"dep_add_index_and_constraint.go",
"dep_drop_column.go",
"dep_drop_constraint.go",
"dep_drop_index.go",
"dep_drop_index_and_column.go",
"dep_drop_object.go",
"dep_garbage_collection.go",
"dep_swap_index.go",
"dep_two_version.go",
"op_drop.go",
"op_index_and_column.go",
"registry.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules/current",
visibility = ["//pkg/sql/schemachanger/scplan:__subpackages__"],
deps = [
"//pkg/sql/schemachanger/rel",
"//pkg/sql/schemachanger/scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen",
"//pkg/sql/schemachanger/scplan/internal/rules",
"//pkg/sql/schemachanger/scplan/internal/scgraph",
"//pkg/sql/schemachanger/screl",
"//pkg/sql/sem/catid",
],
)

go_test(
name = "current_test",
srcs = [
"assertions_test.go",
"rules_test.go",
],
args = ["-test.timeout=295s"],
data = glob(["testdata/**"]),
embed = [":current"],
deps = [
"//pkg/sql/catalog/catpb",
"//pkg/sql/schemachanger/rel",
"//pkg/sql/schemachanger/scpb",
"//pkg/sql/schemachanger/scplan/internal/opgen",
"//pkg/sql/schemachanger/scplan/internal/rules",
"//pkg/sql/schemachanger/screl",
"//pkg/sql/types",
"//pkg/testutils/datapathutils",
"@com_github_cockroachdb_datadriven//:datadriven",
"@com_github_cockroachdb_errors//:errors",
"@com_github_stretchr_testify//require",
"@in_gopkg_yaml_v3//:yaml_v3",
],
)

get_x_data(name = "get_x_data")
29 changes: 15 additions & 14 deletions .../scplan/internal/rules/assertions_test.go → ...internal/rules/current/assertions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package rules
package current

import (
"reflect"
Expand All @@ -19,6 +19,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/opgen"
. "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/rules"
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/errors"
Expand All @@ -41,7 +42,7 @@ func TestRuleAssertions(t *testing.T) {
nameParts := strings.Split(fullName, "rules.")
shortName := nameParts[len(nameParts)-1]
t.Run(shortName, func(t *testing.T) {
_ = forEachElement(func(e scpb.Element) error {
_ = ForEachElement(func(e scpb.Element) error {
e = nonNilElement(e)
if err := fn(e); err != nil {
t.Errorf("%T: %+v", e, err)
Expand All @@ -61,7 +62,7 @@ func nonNilElement(element scpb.Element) scpb.Element {
// One exception is foreign key constraint, which is not simple dependent nor data
// element but it has a screl.ReferencedDescID attribute.
func checkSimpleDependentsReferenceDescID(e scpb.Element) error {
if isSimpleDependent(e) || isData(e) {
if IsSimpleDependent(e) || IsData(e) {
return nil
}
if _, ok := e.(*scpb.ForeignKeyConstraint); ok {
Expand All @@ -84,15 +85,15 @@ func checkToAbsentCategories(e scpb.Element) error {
s1 := opgen.NextStatus(e, scpb.Status_ABSENT, s0)
switch s1 {
case scpb.Status_TXN_DROPPED, scpb.Status_DROPPED:
if IsDescriptor(e) || isData(e) {
if IsDescriptor(e) || IsData(e) {
return nil
}
case scpb.Status_VALIDATED, scpb.Status_WRITE_ONLY, scpb.Status_DELETE_ONLY:
if isSubjectTo2VersionInvariant(e) {
if IsSubjectTo2VersionInvariant(e) {
return nil
}
case scpb.Status_ABSENT:
if isSimpleDependent(e) {
if IsSimpleDependent(e) {
return nil
}
}
Expand All @@ -102,7 +103,7 @@ func checkToAbsentCategories(e scpb.Element) error {
// Assert that isWithTypeT covers all elements with embedded TypeTs.
func checkIsWithTypeT(e scpb.Element) error {
return screl.WalkTypes(e, func(t *types.T) error {
if isWithTypeT(e) {
if IsWithTypeT(e) {
return nil
}
return errors.New("should verify isWithTypeT but doesn't")
Expand All @@ -119,7 +120,7 @@ func checkIsWithExpression(e scpb.Element) error {
case *scpb.RowLevelTTL:
return nil
}
if isWithExpression(e) {
if IsWithExpression(e) {
return nil
}
return errors.New("should verify isWithExpression but doesn't")
Expand All @@ -130,12 +131,12 @@ func checkIsWithExpression(e scpb.Element) error {
// element.
func checkIsColumnDependent(e scpb.Element) error {
// Exclude columns themselves.
if isColumn(e) {
if IsColumn(e) {
return nil
}
// A column dependent should have a ColumnID attribute.
_, err := screl.Schema.GetAttribute(screl.ColumnID, e)
if isColumnDependent(e) {
if IsColumnDependent(e) {
if err != nil {
return errors.New("verifies isColumnDependent but doesn't have ColumnID attr")
}
Expand All @@ -149,12 +150,12 @@ func checkIsColumnDependent(e scpb.Element) error {
// element.
func checkIsIndexDependent(e scpb.Element) error {
// Exclude indexes themselves and their data.
if isIndex(e) || isData(e) || isSupportedNonIndexBackedConstraint(e) {
if IsIndex(e) || IsData(e) || IsSupportedNonIndexBackedConstraint(e) {
return nil
}
// An index dependent should have an IndexID attribute.
_, err := screl.Schema.GetAttribute(screl.IndexID, e)
if isIndexDependent(e) {
if IsIndexDependent(e) {
if err != nil {
return errors.New("verifies isIndexDependent but doesn't have IndexID attr")
}
Expand All @@ -168,12 +169,12 @@ func checkIsIndexDependent(e scpb.Element) error {
// element.
func checkIsConstraintDependent(e scpb.Element) error {
// Exclude constraints themselves.
if isConstraint(e) {
if IsConstraint(e) {
return nil
}
// A constraint dependent should have a ConstraintID attribute.
_, err := screl.Schema.GetAttribute(screl.ConstraintID, e)
if isConstraintDependent(e) {
if IsConstraintDependent(e) {
if err != nil {
return errors.New("verifies isConstraintDependent but doesn't have ConstraintID attr")
}
Expand Down
Loading