sql: Add unit tests for planning inside the new schema changer #65780

fqazi · 2021-05-27T14:51:39Z

Previously, the new schema changer did not have any unit
tests covering the planning capability. This was inadequate,
because we had no way of detect if plans were regressing with
changes or enhancements. To address this, this patch adds
basic tests to see if the operators/dependencies for a given
command are sane.

Release note: None

cockroach-teamcity · 2021-05-27T14:51:45Z

This change is

ajwerner

Reviewed 1 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scplan/plan_test.go, line 138 at r1 (raw file):

Quoted 5 lines of code…

		data, err := json.Marshal(node)
		require.NoError(t, err)
		depMap := make(map[string]interface{})
		err = gojson.Unmarshal(data, &depMap)
		require.NoError(t, err)

Let's replace this all with scgraphviz.toMap

pkg/sql/schemachanger/scplan/testdata/alter_table, line 18 at r1 (raw file):

  name: j
  nullable: true
  pg_attribute_num: 0

Getting these default values is painful. Maybe we should just export scgraphviz.toMap I think that thing does some pretty good stuff pruning the protobuf zero fields and objects.

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/schemachanger/scplan/plan_test.go, line 138 at r1 (raw file):

Previously, ajwerner wrote…

		data, err := json.Marshal(node)
		require.NoError(t, err)
		depMap := make(map[string]interface{})
		err = gojson.Unmarshal(data, &depMap)
		require.NoError(t, err)

Let's replace this all with scgraphviz.toMap

Done.

pkg/sql/schemachanger/scplan/testdata/alter_table, line 18 at r1 (raw file):

Previously, ajwerner wrote…

Getting these default values is painful. Maybe we should just export scgraphviz.toMap I think that thing does some pretty good stuff pruning the protobuf zero fields and objects.

Done.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/schemachanger/scplan/testdata/alter_table, line 8 at r2 (raw file):

ALTER TABLE defaultdb.foo ADD COLUMN j INT
----
Stage 0

What if we did something like below that the eye can see and visually understanding grouping and hierarchy better? It'll be a bit more complex but I don't think it'll be a ton more complex. I'm far from wedded from the below format but I'm trying to make something that looks somewhat like the textual explain. My sense is it will be valuable to have a textual output that we get comfortable with. Anything we do here we can reuse later.

 ├─Stage 0
 │   ├─*scop.MakeAddedColumnDeleteOnly
 │   │    Column:
 │   │        ID: 2
 │   │        Name: j
 │   │        Nullable: true
 │   │        Type:
 │   │            family: IntFamily
 │   │            oid: 20
 │   │            width: 64
 │   │        FamilyName: primary
 │   │    TableID: 52
 │   └─*scop.MakeAddedIndexDeleteOnly
 │        Index:
 │            EncodingType: 1
 │            ID: 2
 │            KeyColumnDirections:
 │            - 0
 │            KeyColumnIDs:
 │            - 1
 │            KeyColumnNames:
 │            - i
 │            Name: new_primary_key
 │            StoreColumnIDs:
 │            - 2
 │            StoreColumnNames:
 │            - j
 │            Unique: true
 │            Version: 3
 │        TableID: 52
 ├─Stage 1
 │   ├─*scop.MakeAddedColumnDeleteAndWriteOnly

ajwerner

Much better.

ajwerner · 2021-05-27T21:20:37Z

pkg/sql/schemachanger/scplan/testdata/alter_table

+ALTER TABLE defaultdb.foo ADD COLUMN j INT
+----
+Stage 0
+	*scop.MakeAddedColumnDeleteOnly


I'm getting nitpicky now: it'd be nice if the yaml indentation levels matched the outer indentation levels.

ajwerner · 2021-05-27T21:21:04Z

pkg/sql/schemachanger/scplan/testdata/drop_sequence

+deps
+DROP SEQUENCE defaultdb.SQ1 CASCADE
+----
+Dependency From:


It'd be nice if this got a similar indentation treatment.

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/schemachanger/scplan/testdata/alter_table, line 9 at r3 (raw file):

Previously, ajwerner wrote…

I'm getting nitpicky now: it'd be nice if the yaml indentation levels matched the outer indentation levels.

Done.

pkg/sql/schemachanger/scplan/testdata/drop_sequence, line 55 at r3 (raw file):

Previously, ajwerner wrote…

It'd be nice if this got a similar indentation treatment.

Done.

ajwerner

nit: lowercase Add in the commit message.

Reviewed 1 of 5 files at r1, 1 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/schemachanger/scplan/testdata/drop_sequence, line 55 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Done.

I think we still want more iteration here. It's hard to see the graph. What if we collect the set of nodes involved in edges, print them much like we print them in the stages thing, and then we reference them with some shorthand.

I think this is a place where the attributes stuff you're adding elsewhere will really come in handy. What if we did it as the set of attributes in a compact representation and then used arrows.

Like imagine the top level was an object with two keys, nodes and edges. Nodes was a map of the "fingerprint" serialization we come up with based on the attributes for the element to the element and then edges is either a map of fingerprint to fingerprint or a list of strings with an arrow between them.
defaultExpression(tableID:52, columnID: 2):ABSENT -> table(tableID: 52):DELETE_ONLY`

pkg/sql/schemachanger/scplan/testdata/drop_sequence, line 56 at r4 (raw file):

----
Dependency From:
  State: 1

it'd be nice to have strings for the states

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/schemachanger/scplan/testdata/drop_sequence, line 56 at r4 (raw file):

Previously, ajwerner wrote…

it'd be nice to have strings for the states

Done.

ajwerner

Reviewed 1 of 3 files at r4, 6 of 6 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scplan/plan_test.go, line 87 at r4 (raw file):


				fallthrough
			case "ops":

Could be another , instead of fallthrough

pkg/sql/schemachanger/scplan/testdata/drop_sequence, line 55 at r5 (raw file):

DROP SEQUENCE defaultdb.SQ1 CASCADE
----
*scpb.DefaultExpression[ Type: 8, DescID: 53, ColumnID: 2 ]:ABSENT->*scpb.Sequence[ Type: 7, DescID: 52 ]:DELETE_ONLY

I think it would also be nice to get the type here. I'd go one further and say that I don't think we need to print the type given it's encoded in 8.

I could also see with this a new format for the Attributes. I'm far from wedded to the [ ... ]. Maybe <type>: {<key>: <value> } so DefaltExpression: {DescID: 53, ColumnID: 2}? That ends up being yaml.

ajwerner

just a couple more minor formatting things and it'll be good to go.

Reviewed 5 of 5 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scpb/attribute.go, line 162 at r6 (raw file):

		}
	}
	result.WriteString(" }")

Can we lose this trailing ?

pkg/sql/schemachanger/scpb/attribute_test.go, line 41 at r6 (raw file):

	// work.
	typeBackRef := TypeReference{TypeID: 3, DescID: 1}
	expectedStr = `*scpb.TypeReference:{DescID: 3, DepID: 1 }`

nit: how do you feel about trimming *scpb in ElementIDToString? I think it'll make some things cleaner.

pkg/sql/schemachanger/scplan/plan_test.go, line 154 at r6 (raw file):

			stages += fmt.Sprintf("%s:%s->%s:%s\n",
				toAttr, de.To().State,
				fromAttr, de.From().State)

What if this also were made to be valid yaml (also moving to strings.Builder).

	var stages string.Builder
	plan.Graph.ForEachNode(func(n *scpb.Node) error {
		return plan.Graph.ForEachDepEdgeFrom(n, func(de *scgraph.DepEdge) error {
			toAttr := de.To().Element().GetAttributes()
			fromAttr := de.From().Element().GetAttributes()
                        fmt.Fprintf(&stages, "- from: [%s, %s]\n", toAttr, de.To().State),
                        fmt.Fprintf(&stages, "  to:   [%s, %s]\n", fromAttr, de.From().State)
                        return nil

Such that what is now:

*scpb.View:{DescID: 54 }:DELETE_ONLY->*scpb.View:{DescID: 53 }:DELETE_ONLY
*scpb.TypeReference:{DescID: 57, DepID: 59 }:ABSENT->*scpb.View:{DescID: 59 }:DELETE_ONLY

will be:

- from: [View:{DescID: 54}, DELETE_ONLY]
  to:   [View:{DescID: 53}, DELETE_ONLY]
- from: [TypeReference:{DescID: 57, DepID: 59}, ABSENT]
  to:   [View:{DescID: 59}, DELETE_ONLY]

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/schemachanger/scpb/attribute.go, line 162 at r6 (raw file):

Previously, ajwerner wrote…

Can we lose this trailing ?

Done.

pkg/sql/schemachanger/scplan/plan_test.go, line 154 at r6 (raw file):

Previously, ajwerner wrote…

			stages += fmt.Sprintf("%s:%s->%s:%s\n",
				toAttr, de.To().State,
				fromAttr, de.From().State)

What if this also were made to be valid yaml (also moving to strings.Builder).

	var stages string.Builder
	plan.Graph.ForEachNode(func(n *scpb.Node) error {
		return plan.Graph.ForEachDepEdgeFrom(n, func(de *scgraph.DepEdge) error {
			toAttr := de.To().Element().GetAttributes()
			fromAttr := de.From().Element().GetAttributes()
                        fmt.Fprintf(&stages, "- from: [%s, %s]\n", toAttr, de.To().State),
                        fmt.Fprintf(&stages, "  to:   [%s, %s]\n", fromAttr, de.From().State)
                        return nil

Such that what is now:

*scpb.View:{DescID: 54 }:DELETE_ONLY->*scpb.View:{DescID: 53 }:DELETE_ONLY
*scpb.TypeReference:{DescID: 57, DepID: 59 }:ABSENT->*scpb.View:{DescID: 59 }:DELETE_ONLY

will be:

- from: [View:{DescID: 54}, DELETE_ONLY]
  to:   [View:{DescID: 53}, DELETE_ONLY]
- from: [TypeReference:{DescID: 57, DepID: 59}, ABSENT]
  to:   [View:{DescID: 59}, DELETE_ONLY]

Done.

ajwerner

Reviewed 6 of 6 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @fqazi)

pkg/sql/schemachanger/scpb/elements.go, line 80 at r7 (raw file):

		}
		typeToElementID[f.Type] = ElementTypeID(id)
		elementIDToString[ElementTypeID(id)] = strings.ReplaceAll(f.Type.String(), "*scpb.", "")

nit: strings.TrimPrefix

fqazi · 2021-06-03T18:59:54Z

TFTR @ajwerner

fqazi · 2021-06-03T19:00:29Z

bors r=ajwerner

fqazi · 2021-06-04T02:46:52Z

bors r=ajwerner

craig · 2021-06-04T03:03:58Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-06-04T05:15:23Z

Build failed:

GitHub CI (Cockroach)

fqazi · 2021-06-04T16:11:23Z

bors r=ajwerner

craig · 2021-06-04T16:11:25Z

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

craig · 2021-06-04T16:43:03Z

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

fqazi · 2021-06-04T16:44:04Z

bors r=ajwerner

craig · 2021-06-04T16:44:07Z

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

craig · 2021-06-04T17:15:45Z

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

ajwerner · 2021-06-04T17:30:24Z

darn cla bot seems sad. https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73203936/CLA+Assistant+glitch+in+PRs or push a no-op commit

Previously, the new schema changer did not have any unit tests covering the planning capability. This was inadequate, because we had no way of detect if plans were regressing with changes or enhancements. To address this, this patch adds basic tests to see if the operators/dependencies for a given command are sane. Release note: None

fqazi · 2021-06-04T17:39:50Z

bors r=ajwerner

fqazi · 2021-06-04T17:40:02Z

darn cla bot seems sad. https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73203936/CLA+Assistant+glitch+in+PRs or push a no-op commit

Thanks @ajwerner

craig · 2021-06-04T18:58:41Z

Build succeeded:

GitHub CI (Cockroach)

fqazi requested a review from a team May 27, 2021 14:51

ajwerner reviewed May 27, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 8f70402 to 1a0e5ea Compare May 27, 2021 19:30

fqazi commented May 27, 2021

View reviewed changes

fqazi requested a review from ajwerner May 27, 2021 19:32

ajwerner reviewed May 27, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 1a0e5ea to 1163b32 Compare May 27, 2021 21:12

fqazi requested a review from ajwerner May 27, 2021 21:12

ajwerner reviewed May 27, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 1163b32 to 38531fa Compare May 28, 2021 13:43

fqazi requested a review from ajwerner May 28, 2021 13:43

fqazi commented May 28, 2021

View reviewed changes

ajwerner reviewed May 28, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 38531fa to 7a2e44b Compare June 2, 2021 14:18

fqazi requested a review from ajwerner June 2, 2021 14:19

fqazi commented Jun 2, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 7a2e44b to a150367 Compare June 3, 2021 01:29

ajwerner reviewed Jun 3, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from a150367 to 159668a Compare June 3, 2021 14:30

fqazi requested a review from ajwerner June 3, 2021 14:30

ajwerner reviewed Jun 3, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 159668a to 0e9a34a Compare June 3, 2021 18:11

fqazi commented Jun 3, 2021

View reviewed changes

fqazi requested a review from ajwerner June 3, 2021 18:12

ajwerner approved these changes Jun 3, 2021

View reviewed changes

fqazi force-pushed the scPlanTesting branch from 0e9a34a to 590003e Compare June 3, 2021 18:59

cockroachdb deleted a comment from craig bot Jun 4, 2021

fqazi force-pushed the scPlanTesting branch 4 times, most recently from 728cc69 to 5438f3b Compare June 4, 2021 14:21

fqazi force-pushed the scPlanTesting branch from 5438f3b to 721d403 Compare June 4, 2021 16:10

cockroachdb deleted a comment from craig bot Jun 4, 2021

fqazi force-pushed the scPlanTesting branch from 721d403 to a6ecdea Compare June 4, 2021 17:39

craig bot merged commit 02830c2 into cockroachdb:master Jun 4, 2021

fqazi deleted the scPlanTesting branch June 4, 2021 18:58

fqazi mentioned this pull request Jun 7, 2021

scplan: add datadriven tests similar to the scbuild tests #65702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Add unit tests for planning inside the new schema changer #65780

sql: Add unit tests for planning inside the new schema changer #65780

fqazi commented May 27, 2021

cockroach-teamcity commented May 27, 2021

ajwerner left a comment

fqazi left a comment

ajwerner left a comment •

edited

Loading

ajwerner left a comment

ajwerner May 27, 2021

ajwerner May 27, 2021

fqazi left a comment

ajwerner left a comment

fqazi left a comment

ajwerner left a comment

ajwerner left a comment

fqazi left a comment

ajwerner left a comment

fqazi commented Jun 3, 2021

fqazi commented Jun 3, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

ajwerner commented Jun 4, 2021

fqazi commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

sql: Add unit tests for planning inside the new schema changer #65780

sql: Add unit tests for planning inside the new schema changer #65780

Conversation

fqazi commented May 27, 2021

cockroach-teamcity commented May 27, 2021

ajwerner left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment • edited Loading

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner May 27, 2021

Choose a reason for hiding this comment

ajwerner May 27, 2021

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

fqazi commented Jun 3, 2021

fqazi commented Jun 3, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

craig bot commented Jun 4, 2021

ajwerner commented Jun 4, 2021

fqazi commented Jun 4, 2021

fqazi commented Jun 4, 2021

craig bot commented Jun 4, 2021

ajwerner left a comment •

edited

Loading