Skip to content

Commit cd90b8a

Browse files
Xiang-GuXiang-Gu
committed
compose: Deflake TestComposeCompare
This commit fixes a previously overlooked issue to actually disable using locales when creating tables in the TestComposeCompare test. Release Note: None
1 parent 63a18f9 commit cd90b8a

File tree

2 files changed

+150
-126
lines changed

2 files changed

+150
-126
lines changed

‎pkg/sql/randgen/mutator.go‎

Lines changed: 108 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -735,127 +735,131 @@ func postgresCreateTableMutator(
735735
) (mutated []tree.Statement, changed bool) {
736736
for _, stmt := range stmts {
737737
mutated = append(mutated, stmt)
738-
switch stmt := stmt.(type) {
739-
case *tree.CreateTable:
740-
// Get all the column types first.
741-
colTypes := make(map[string]*types.T)
742-
for _, def := range stmt.Defs {
743-
switch def := def.(type) {
744-
case *tree.ColumnTableDef:
745-
colType := tree.MustBeStaticallyKnownType(def.Type)
746-
// Disable locales because CRDB random generator generates
747-
// locales without double-quotes, and they are unknown to
748-
// Postgres (e.g. `i TEXT COLLATE de_DE` won't work with
749-
// Postgres).
750-
if colType.Family() == types.CollatedStringFamily {
751-
colType = types.String
752-
}
753-
colTypes[string(def.Name)] = colType
754-
}
738+
mutatedStmt, ok := stmt.(*tree.CreateTable)
739+
if !ok {
740+
continue
741+
}
742+
743+
// Get all the column types first.
744+
colTypes := make(map[string]*types.T)
745+
for _, def := range mutatedStmt.Defs {
746+
def, ok := def.(*tree.ColumnTableDef)
747+
if !ok {
748+
continue
755749
}
750+
colDefType := tree.MustBeStaticallyKnownType(def.Type)
751+
colTypes[string(def.Name)] = colDefType
752+
}
756753

757-
// Exclude `INDEX` and `UNIQUE` table defs and hoist them into separate
758-
// `CREATE INDEX` and `CREATE UNIQUE INDEX` statements.
759-
var newdefs tree.TableDefs
760-
for _, def := range stmt.Defs {
761-
switch def := def.(type) {
762-
case *tree.IndexTableDef:
763-
// Postgres doesn't support indexes in CREATE TABLE, so split them out
764-
// to their own statement.
765-
var newCols tree.IndexElemList
766-
for _, col := range def.Columns {
767-
isBox2d := false
768-
// NB: col.Column is empty for expression-based indexes.
769-
if col.Expr == nil {
770-
// Postgres doesn't support box2d as a btree index key.
771-
colTypeFamily := colTypes[string(col.Column)].Family()
772-
if colTypeFamily == types.Box2DFamily {
773-
isBox2d = true
774-
}
775-
}
776-
if isBox2d {
777-
changed = true
778-
} else {
779-
newCols = append(newCols, col)
780-
}
781-
}
782-
if len(newCols) == 0 {
783-
// Break without adding this index at all.
784-
break
785-
}
786-
def.Columns = newCols
787-
// Hoist this IndexTableDef into a separate CreateIndex.
788-
changed = true
789-
// TODO(rafi): Postgres supports inverted indexes with a different
790-
// syntax than Cockroach. Maybe we could add it later.
791-
// The syntax is `CREATE INDEX name ON table USING gin(column)`.
792-
if !def.Inverted {
793-
mutated = append(mutated, &tree.CreateIndex{
794-
Name: def.Name,
795-
Table: stmt.Table,
796-
Inverted: def.Inverted,
797-
Columns: newCols,
798-
Storing: def.Storing,
799-
// Postgres doesn't support NotVisible Index, so NotVisible is not populated here.
800-
})
801-
}
802-
case *tree.UniqueConstraintTableDef:
803-
var newCols tree.IndexElemList
804-
for _, col := range def.Columns {
805-
isBox2d := false
806-
// NB: col.Column is empty for expression-based indexes.
807-
if col.Expr == nil {
808-
// Postgres doesn't support box2d as a btree index key.
809-
colTypeFamily := colTypes[string(col.Column)].Family()
810-
if colTypeFamily == types.Box2DFamily {
811-
isBox2d = true
812-
}
813-
}
814-
if isBox2d {
815-
changed = true
816-
} else {
817-
newCols = append(newCols, col)
754+
// - Exclude `INDEX` and `UNIQUE` table defs and hoist them into separate
755+
// `CREATE INDEX` and `CREATE UNIQUE INDEX` statements because Postgres does
756+
// not support them in `CREATE TABLE` stmt.
757+
// - Erase `COLLATE locale` from column defs because Postgres only support
758+
// double-quoted locale.
759+
var newdefs tree.TableDefs
760+
for _, def := range mutatedStmt.Defs {
761+
switch def := def.(type) {
762+
case *tree.IndexTableDef:
763+
var newCols tree.IndexElemList
764+
for _, col := range def.Columns {
765+
isBox2d := false
766+
// NB: col.Column is empty for expression-based indexes.
767+
if col.Expr == nil {
768+
// Postgres doesn't support box2d as a btree index key.
769+
colTypeFamily := colTypes[string(col.Column)].Family()
770+
if colTypeFamily == types.Box2DFamily {
771+
isBox2d = true
818772
}
819773
}
820-
if len(newCols) == 0 {
821-
// Break without adding this index at all.
822-
break
823-
}
824-
def.Columns = newCols
825-
if def.PrimaryKey {
826-
for i, col := range def.Columns {
827-
// Postgres doesn't support descending PKs.
828-
if col.Direction != tree.DefaultDirection {
829-
def.Columns[i].Direction = tree.DefaultDirection
830-
changed = true
831-
}
832-
}
833-
if def.Name != "" {
834-
// Unset Name here because constraint names cannot be shared among
835-
// tables, so multiple PK constraints named "primary" is an error.
836-
def.Name = ""
837-
changed = true
838-
}
839-
newdefs = append(newdefs, def)
840-
break
774+
if isBox2d {
775+
changed = true
776+
} else {
777+
newCols = append(newCols, col)
841778
}
779+
}
780+
if len(newCols) == 0 {
781+
// Break without adding this index at all.
782+
break
783+
}
784+
def.Columns = newCols
785+
// Hoist this IndexTableDef into a separate CreateIndex.
786+
changed = true
787+
// TODO(rafi): Postgres supports inverted indexes with a different
788+
// syntax than Cockroach. Maybe we could add it later.
789+
// The syntax is `CREATE INDEX name ON table USING gin(column)`.
790+
if !def.Inverted {
842791
mutated = append(mutated, &tree.CreateIndex{
843792
Name: def.Name,
844-
Table: stmt.Table,
845-
Unique: true,
793+
Table: mutatedStmt.Table,
846794
Inverted: def.Inverted,
847795
Columns: newCols,
848796
Storing: def.Storing,
849797
// Postgres doesn't support NotVisible Index, so NotVisible is not populated here.
850798
})
851-
changed = true
852-
default:
799+
}
800+
case *tree.UniqueConstraintTableDef:
801+
var newCols tree.IndexElemList
802+
for _, col := range def.Columns {
803+
isBox2d := false
804+
// NB: col.Column is empty for expression-based indexes.
805+
if col.Expr == nil {
806+
// Postgres doesn't support box2d as a btree index key.
807+
colTypeFamily := colTypes[string(col.Column)].Family()
808+
if colTypeFamily == types.Box2DFamily {
809+
isBox2d = true
810+
}
811+
}
812+
if isBox2d {
813+
changed = true
814+
} else {
815+
newCols = append(newCols, col)
816+
}
817+
}
818+
if len(newCols) == 0 {
819+
// Break without adding this index at all.
820+
break
821+
}
822+
def.Columns = newCols
823+
if def.PrimaryKey {
824+
for i, col := range def.Columns {
825+
// Postgres doesn't support descending PKs.
826+
if col.Direction != tree.DefaultDirection {
827+
def.Columns[i].Direction = tree.DefaultDirection
828+
changed = true
829+
}
830+
}
831+
if def.Name != "" {
832+
// Unset Name here because constraint names cannot be shared among
833+
// tables, so multiple PK constraints named "primary" is an error.
834+
def.Name = ""
835+
changed = true
836+
}
853837
newdefs = append(newdefs, def)
838+
break
854839
}
840+
mutated = append(mutated, &tree.CreateIndex{
841+
Name: def.Name,
842+
Table: mutatedStmt.Table,
843+
Unique: true,
844+
Inverted: def.Inverted,
845+
Columns: newCols,
846+
Storing: def.Storing,
847+
// Postgres doesn't support NotVisible Index, so NotVisible is not populated here.
848+
})
849+
changed = true
850+
case *tree.ColumnTableDef:
851+
if def.Type.(*types.T).Family() == types.CollatedStringFamily {
852+
def.Type = types.String
853+
changed = true
854+
}
855+
newdefs = append(newdefs, def)
856+
default:
857+
newdefs = append(newdefs, def)
855858
}
856-
stmt.Defs = newdefs
857859
}
860+
mutatedStmt.Defs = newdefs
858861
}
862+
859863
return mutated, changed
860864
}
861865

‎pkg/sql/randgen/mutator_test.go‎

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/sql/randgen"
1818
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
1919
"github.com/cockroachdb/cockroach/pkg/util/randutil"
20+
"github.com/stretchr/testify/require"
2021
)
2122

2223
func TestPostgresMutator(t *testing.T) {
2324
defer leaktest.AfterTest(t)()
25+
rng, _ := randutil.NewTestRand()
2426
q := `
25-
CREATE TABLE t (s STRING FAMILY fam1, b BYTES, FAMILY fam2 (b), PRIMARY KEY (s ASC, b DESC), INDEX (s) STORING (b))
27+
CREATE TABLE t (s STRING FAMILY fam1, b BYTES, FAMILY fam2 (b), PRIMARY KEY (s ASC, b DESC), INDEX (s) STORING (b), c TEXT COLLATE en_US NOT NULL)
2628
PARTITION BY LIST (s)
2729
(
2830
PARTITION europe_west VALUES IN ('a', 'b')
@@ -31,27 +33,45 @@ func TestPostgresMutator(t *testing.T) {
3133
SET CLUSTER SETTING "sql.stats.automatic_collection.enabled" = false;
3234
`
3335

34-
rng, _ := randutil.NewTestRand()
35-
{
36-
mutated, changed := randgen.ApplyString(rng, q, randgen.PostgresMutator)
37-
if !changed {
38-
t.Fatal("expected changed")
39-
}
40-
mutated = strings.TrimSpace(mutated)
41-
expect := `CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s ASC, b DESC), INDEX (s) INCLUDE (b));`
42-
if mutated != expect {
43-
t.Fatalf("unexpected: %s", mutated)
44-
}
36+
type TestCase struct {
37+
name string
38+
// original statement(s) string and mutators to apply
39+
original string
40+
mutators []randgen.Mutator
41+
// mutated after applying mutators
42+
mutated string
43+
changed bool
4544
}
46-
{
47-
mutated, changed := randgen.ApplyString(rng, q, randgen.PostgresCreateTableMutator, randgen.PostgresMutator)
48-
if !changed {
49-
t.Fatal("expected changed")
50-
}
51-
mutated = strings.TrimSpace(mutated)
52-
expect := "CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s, b));\nCREATE INDEX ON t (s) INCLUDE (b);"
53-
if mutated != expect {
54-
t.Fatalf("unexpected: %s", mutated)
55-
}
45+
46+
for _, testCase := range []TestCase{
47+
{
48+
name: "postgresCreateTableMutator",
49+
original: q,
50+
mutators: []randgen.Mutator{randgen.PostgresCreateTableMutator},
51+
mutated: "CREATE TABLE t (s STRING FAMILY fam1, b BYTES, FAMILY fam2 (b), PRIMARY KEY (s, b), c STRING NOT NULL) PARTITION BY LIST (s) (PARTITION europe_west VALUES IN ('a', 'b'));\nCREATE INDEX ON t (s) STORING (b);\nALTER TABLE table1 INJECT STATISTICS 'blah';\nSET CLUSTER SETTING \"sql.stats.automatic_collection.enabled\" = false;",
52+
changed: true,
53+
},
54+
{
55+
name: "postgresMutator",
56+
original: q,
57+
mutators: []randgen.Mutator{randgen.PostgresMutator},
58+
mutated: `CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s ASC, b DESC), INDEX (s) INCLUDE (b), c TEXT COLLATE en_US NOT NULL);`,
59+
changed: true,
60+
},
61+
{
62+
name: "postgresCreateTableMutator + postgresMutator",
63+
original: q,
64+
mutators: []randgen.Mutator{randgen.PostgresCreateTableMutator, randgen.PostgresMutator},
65+
mutated: "CREATE TABLE t (s TEXT, b BYTEA, PRIMARY KEY (s, b), c TEXT NOT NULL);\nCREATE INDEX ON t (s) INCLUDE (b);",
66+
changed: true,
67+
},
68+
{},
69+
} {
70+
t.Run(testCase.name, func(t *testing.T) {
71+
actual, changed := randgen.ApplyString(rng, testCase.original, testCase.mutators...)
72+
require.Equal(t, testCase.changed, changed, "expected changed=%v; get %v", testCase.changed, changed)
73+
actual = strings.TrimSpace(actual)
74+
require.Equal(t, testCase.mutated, actual, "expected mutated = %v; get %v", testCase.mutated, actual)
75+
})
5676
}
5777
}

0 commit comments

Comments
 (0)