Skip to content

execbuilder: re-enable skipped tests#75972

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:75960-re-enable-flakes
Feb 4, 2022
Merged

execbuilder: re-enable skipped tests#75972
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:75960-re-enable-flakes

Conversation

@mgartner
Copy link
Copy Markdown
Contributor

@mgartner mgartner commented Feb 3, 2022

This commit de-flakes tests that were previously disabled in #75962 by
setting column families explicitly. I originally thought these
execbuilder tests were flaky because there were two query plans with the
same cost, but the flakiness was actually the result of randomized
families in logic tests.

Fixes #75960

Release note: None

@mgartner
execbuilder: re-enable skipped tests
d1e3916 
This commit de-flakes tests that were previously disabled in cockroachdb#75962 by
setting column families explicitly. I originally thought these
execbuilder tests were flaky because there were two query plans with the
same cost, but the flakiness was actually the result of randomized
families in logic tests.

Fixes cockroachdb#75960

Release note: None
@mgartner mgartner requested review from a team and RaduBerinde February 3, 2022 19:27
@mgartner mgartner requested a review from a team as a code owner February 3, 2022 19:27
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Feb 3, 2022

This change is Reviewable

cucaroach
cucaroach approved these changes Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

cucaroach
cucaroach approved these changes Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

rharding6373
rharding6373 reviewed Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 203 at r1 (raw file):

  a INT8 PRIMARY KEY USING HASH WITH BUCKET_COUNT = 8,
  b INT8 NOT NULL,
  FAMILY (a, b)

Drive-by question: So without specifying the column family, we will store all columns as a single family but randomly decide whether to store them in (a,b) order or (b,a) order?

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 3, 2022

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 203 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Drive-by question: So without specifying the column family, we will store all columns as a single family but randomly decide whether to store them in (a,b) order or (b,a) order?

The logic tests use a column family mutator here:

cockroach/pkg/sql/logictest/logic.go

Line 2791 in 73b6819

execSQL, changed := randgen.ApplyString(t.rng, stmt.sql, randgen.ColumnFamilyMutator)

This mutates CREATE TABLE statements by randomly splitting up columns into a random number of families if there are no families explicitly specified in the CREATE TABLE statement:

cockroach/pkg/sql/randgen/mutator.go

Lines 821 to 870 in 8b5f53c

func columnFamilyMutator(rng *rand.Rand, stmt tree.Statement) (changed bool) {
ast, ok := stmt.(*tree.CreateTable)
if !ok {
return false
}
var columns []tree.Name
for _, def := range ast.Defs {
switch def := def.(type) {
case *tree.FamilyTableDef:
return false
case *tree.ColumnTableDef:
if def.HasColumnFamily() {
return false
}
if !def.Computed.Virtual {
columns = append(columns, def.Name)
}
}
}
if len(columns) <= 1 {
return false
}
// Any columns not specified in column families
// are auto assigned to the first family, so
// there's no requirement to exhaust columns here.
rng.Shuffle(len(columns), func(i, j int) {
columns[i], columns[j] = columns[j], columns[i]
})
fd := &tree.FamilyTableDef{}
for {
if len(columns) == 0 {
if len(fd.Columns) > 0 {
ast.Defs = append(ast.Defs, fd)
}
break
}
fd.Columns = append(fd.Columns, columns[0])
columns = columns[1:]
// 50% chance to make a new column family.
if rng.Intn(2) != 0 {
ast.Defs = append(ast.Defs, fd)
fd = &tree.FamilyTableDef{}
}
}
return true
}

This is only happens for logic tests. In production, column families are always deterministic. I believe the default is to include all columns in a single family.

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 3, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 4, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 4, 2022

Build failed:

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 4, 2022

Looks like a flake.

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 4, 2022

Build succeeded:

@craig craig bot merged commit b51c9b4 into cockroachdb:master Feb 4, 2022
@mgartner mgartner deleted the 75960-re-enable-flakes branch February 4, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rharding6373 rharding6373 rharding6373 left review comments

@RaduBerinde RaduBerinde Awaiting requested review from RaduBerinde

+1 more reviewer

@cucaroach cucaroach cucaroach approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sql: TestExecBuild/local/hash_sharded_index/test_hash_index_unique_constraint_sec_key fails under stress

4 participants

@mgartner @cockroach-teamcity @rharding6373 @cucaroach