diff --git a/pkg/sql/schemaexpr/check_constraint.go b/pkg/sql/schemaexpr/check_constraint.go index e8be68eab2f..36a26dcb541 100644 --- a/pkg/sql/schemaexpr/check_constraint.go +++ b/pkg/sql/schemaexpr/check_constraint.go @@ -72,10 +72,8 @@ func (b *CheckConstraintBuilder) MarkNameInUse(name string) { // - It does not include subqueries. // - It does not include aggregate, window, or set returning functions. // -// Note that mutable functions are currently allowed, but using them can lead -// to unexpected behavior. -// TODO(mgartner): Add unit tests for Build. Some cases should test -// MarkNameInUse, too. +// Note that mutable functions are currently allowed, unlike in partial index +// predicates, but using them can lead to unexpected behavior. func (b *CheckConstraintBuilder) Build( c *tree.CheckConstraintTableDef, ) (*sqlbase.TableDescriptor_CheckConstraint, error) { @@ -184,7 +182,6 @@ func (b *CheckConstraintBuilder) generateUniqueName(expr tree.Expr) (string, err // // Note that the generated name is not guaranteed to be unique among the other // constraints of the table. -// TODO(mgartner): Add unit tests for DefaultName. func (b *CheckConstraintBuilder) DefaultName(expr tree.Expr) (string, error) { var nameBuf bytes.Buffer nameBuf.WriteString("check") diff --git a/pkg/sql/schemaexpr/check_constraint_test.go b/pkg/sql/schemaexpr/check_constraint_test.go new file mode 100644 index 00000000000..a3c15028d2f --- /dev/null +++ b/pkg/sql/schemaexpr/check_constraint_test.go @@ -0,0 +1,193 @@ +// Copyright 2020 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 schemaexpr + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/sql/parser" + "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/types" +) + +func TestCheckConstraintBuilder_MarkNameInUse(t *testing.T) { + // TODO(mgartner); +} + +func TestCheckConstraintBuilder_Build(t *testing.T) { + ctx := context.Background() + semaCtx := tree.MakeSemaContext() + + // Trick to get the init() for the builtins package to run. + _ = builtins.AllBuiltinNames + + database := tree.Name("foo") + table := tree.Name("bar") + tn := tree.MakeTableName(database, table) + + desc := testTableDesc( + string(table), + []testCol{{"a", types.Bool}, {"b", types.Int}}, + []testCol{{"c", types.String}}, + ) + + builder := NewCheckConstraintBuilder(ctx, tn, &desc, &semaCtx) + builder.MarkNameInUse("check_a3") + + testData := []struct { + name string + expr string + expectedValid bool + expectedExpr string + expectedName string + }{ + // Respect custom names. + {"chk_1", "a", true, "a", "chk_1"}, + + // Use unique default names when there is no custom name. + {"", "a", true, "a", "check_a"}, + {"", "a", true, "a", "check_a1"}, + {"", "a", true, "a", "check_a2"}, + {"", "a AND b = 0", true, "a AND (b = 0)", "check_a_b"}, + {"", "a AND b = 1", true, "a AND (b = 1)", "check_a_b1"}, + {"", "a AND b = 1", true, "a AND (b = 1)", "check_a_b2"}, + + // Respect that "check_a3" has been marked, so the next check constraint + // with "a" should be "check_a4". + {"", "a", true, "a", "check_a4"}, + {"", "a", true, "a", "check_a5"}, + + // Allow expressions that result in a bool. + {"ck", "a", true, "a", "ck"}, + {"ck", "b = 0", true, "b = 0", "ck"}, + {"ck", "a AND b = 0", true, "a AND (b = 0)", "ck"}, + {"ck", "a IS NULL", true, "a IS NULL", "ck"}, + {"ck", "b IN (1, 2)", true, "b IN (1:::INT8, 2:::INT8)", "ck"}, + + // Allow immutable functions. + {"ck", "abs(b) > 0", true, "abs(b) > 0", "ck"}, + {"ck", "c || c = 'foofoo'", true, "(c || c) = 'foofoo'", "ck"}, + {"ck", "lower(c) = 'bar'", true, "lower(c) = 'bar'", "ck"}, + + // Allow mutable functions. + {"ck", "b > random()", true, "b > random()", "ck"}, + + // Disallow references to columns not in the table. + {"", "d", false, "", ""}, + {"", "t.a", false, "", ""}, + + // Disallow expressions that do not result in a bool. + {"", "b", false, "", ""}, + {"", "abs(b)", false, "", ""}, + {"", "lower(c)", false, "", ""}, + + // Disallow subqueries. + {"", "exists(select 1)", false, "", ""}, + {"", "b IN (select 1)", false, "", ""}, + + // Disallow aggregate, window, and set returning functions. + {"", "sum(b) > 10", false, "", ""}, + {"", "row_number() OVER () > 1", false, "", ""}, + {"", "generate_series(1, 1) > 2", false, "", ""}, + + // Dequalify column names. + {"ck", "bar.a", true, "a", "ck"}, + {"ck", "foo.bar.a", true, "a", "ck"}, + {"ck", "bar.b = 0", true, "b = 0", "ck"}, + {"ck", "foo.bar.b = 0", true, "b = 0", "ck"}, + {"ck", "bar.a AND foo.bar.b = 0", true, "a AND (b = 0)", "ck"}, + } + + for _, d := range testData { + t.Run(d.expr, func(t *testing.T) { + expr, err := parser.ParseExpr(d.expr) + if err != nil { + t.Fatalf("%s: unexpected error: %s", d.expr, err) + } + + ckDef := &tree.CheckConstraintTableDef{Name: tree.Name(d.name), Expr: expr} + + ck, err := builder.Build(ckDef) + + if !d.expectedValid { + if err == nil { + t.Fatalf("%s: expected invalid expression, but was valid", d.expr) + } + // The input expression is invalid so there is no need to check + // the output ck. + return + } + + if err != nil { + t.Fatalf("%s: expected valid expression, but found error: %s", d.expr, err) + } + + if ck.Name != d.expectedName || ck.Expr != d.expectedExpr { + t.Errorf( + `%s: expected "%s CHECK %s", got "%s CHECK %s"`, + d.expr, + d.expectedName, + d.expectedExpr, + ck.Name, + ck.Expr, + ) + } + }) + } +} + +func TestCheckConstraintBuilder_DefaultName(t *testing.T) { + ctx := context.Background() + semaCtx := tree.MakeSemaContext() + + database := tree.Name("foo") + table := tree.Name("bar") + tn := tree.MakeTableName(database, table) + + desc := testTableDesc( + string(table), + []testCol{{"a", types.Bool}, {"b", types.Int}}, + []testCol{{"c", types.String}, {"d", types.String}, {"e", types.String}}, + ) + + builder := NewCheckConstraintBuilder(ctx, tn, &desc, &semaCtx) + + testData := []struct { + expr string + expected string + }{ + {"a > 0", "check_a"}, + {"a > 0 AND b = 'foo'", "check_a_b"}, + {"a > 0 AND (b = 'foo' OR a < 20)", "check_a_b_a"}, + {"a > 0 AND a < 20", "check_a_a"}, + {"((a AND (b OR c)) AND d) OR e", "check_a_b_c_d_e"}, + } + + for _, d := range testData { + t.Run(d.expr, func(t *testing.T) { + expr, err := parser.ParseExpr(d.expr) + if err != nil { + t.Fatalf("%s: unexpected error: %s", d.expr, err) + } + + r, err := builder.DefaultName(expr) + if err != nil { + t.Fatalf("%s: expected success, but found error: %s", d.expr, err) + } + + if r != d.expected { + t.Errorf("%s: expected %q, got %q", d.expr, d.expected, r) + } + }) + } +} diff --git a/pkg/sql/schemaexpr/partial_index_test.go b/pkg/sql/schemaexpr/partial_index_test.go index e2ec12b8e98..f487a9e0dc9 100644 --- a/pkg/sql/schemaexpr/partial_index_test.go +++ b/pkg/sql/schemaexpr/partial_index_test.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -32,36 +31,11 @@ func TestIndexPredicateValidator_Validate(t *testing.T) { table := tree.Name("bar") tn := tree.MakeTableName(database, table) - desc := sqlbase.MutableTableDescriptor{ - TableDescriptor: sqlbase.TableDescriptor{ - Name: string(table), - ID: 1, - Columns: []sqlbase.ColumnDescriptor{ - { - Name: "a", - ID: 1, - Type: types.Bool, - }, - { - Name: "b", - ID: 2, - Type: types.Int, - }, - }, - Mutations: []sqlbase.DescriptorMutation{ - { - Descriptor_: &sqlbase.DescriptorMutation_Column{ - Column: &sqlbase.ColumnDescriptor{ - Name: "c", - ID: 3, - Type: types.String, - }, - }, - Direction: sqlbase.DescriptorMutation_ADD, - }, - }, - }, - } + desc := testTableDesc( + string(table), + []testCol{{"a", types.Bool}, {"b", types.Int}}, + []testCol{{"c", types.String}}, + ) validator := NewIndexPredicateValidator(ctx, tn, &desc, &semaCtx) diff --git a/pkg/sql/schemaexpr/testutils.go b/pkg/sql/schemaexpr/testutils.go new file mode 100644 index 00000000000..427d4df6ac0 --- /dev/null +++ b/pkg/sql/schemaexpr/testutils.go @@ -0,0 +1,51 @@ +package schemaexpr + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" + "github.com/cockroachdb/cockroach/pkg/sql/types" +) + +// testCol includes the information needed to create a column descriptor for +// testing purposes. +type testCol struct { + name string + typ *types.T +} + +// testTableDesc is a helper functions for creating table descriptors in a +// less verbose way. +func testTableDesc( + name string, columns []testCol, mutationColumns []testCol, +) sqlbase.MutableTableDescriptor { + cols := make([]sqlbase.ColumnDescriptor, len(columns)) + for i := range columns { + cols[i] = sqlbase.ColumnDescriptor{ + Name: columns[i].name, + Type: columns[i].typ, + ID: sqlbase.ColumnID(i), + } + } + + muts := make([]sqlbase.DescriptorMutation, len(mutationColumns)) + for i := range mutationColumns { + muts[i] = sqlbase.DescriptorMutation{ + Descriptor_: &sqlbase.DescriptorMutation_Column{ + Column: &sqlbase.ColumnDescriptor{ + Name: mutationColumns[i].name, + Type: mutationColumns[i].typ, + ID: sqlbase.ColumnID(len(columns) + i), + }, + }, + Direction: sqlbase.DescriptorMutation_ADD, + } + } + + return sqlbase.MutableTableDescriptor{ + TableDescriptor: sqlbase.TableDescriptor{ + Name: name, + ID: 1, + Columns: cols, + Mutations: muts, + }, + } +}