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.

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

sqlsmith: add support for computed columns #61491

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

RaduBerinde
Copy link
Member

This changes the random table generator to also create computed
columns (either STORED or VIRTUAL). Some example of definitions:

  • col1_14 STRING NOT NULL AS (lower(CAST(col1_8 AS STRING))) VIRTUAL
  • col2_6 DECIMAL NOT NULL AS (col2_2 + 1:::DECIMAL) STORED
  • col1_13 INT4 AS (col1_0 + col1_10) STORED

Release justification: non-production code change.

Release note: None

Informs #57608.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner 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 @jordanlewis and @RaduBerinde)


pkg/sql/rowenc/testutils.go, line 1245 at r1 (raw file):

		// Make defs for computed columns.
		normalColDefs := columnDefs
		for i := nNormalColumns; i < nColumns; i++ {

[nit] i is not used in the block, so for i := 0; i < nComputedColumns; i++ { might be easier to understand.


pkg/sql/rowenc/testutils.go, line 1536 at r1 (raw file):

	if rng.Intn(2) == 0 {
		// Try to find a set of numeric columns with the same type so we can add
		// them together as the computed expression.

[nit] expand this explanation similar to the single-column explanation below. An example might be nice.


pkg/sql/rowenc/testutils.go, line 1594 at r1 (raw file):

		case types.DecimalFamily:
			var err error
			right, err = tree.ParseDDecimal("1")

[nit] I think you could use RandDatum to get a random "special" datum for right, rather than hard-coding to 1.

Copy link
Collaborator

@mgartner mgartner 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 @jordanlewis and @RaduBerinde)


pkg/sql/rowenc/testutils.go, line 1245 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

[nit] i is not used in the block, so for i := 0; i < nComputedColumns; i++ { might be easier to understand.

nvm, i is used. disregard.

@RaduBerinde RaduBerinde force-pushed the sqlsmith-virtual-cols branch 2 times, most recently from a395586 to 6a8b39a Compare March 4, 2021 19:07
Copy link
Member Author

@RaduBerinde RaduBerinde 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! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis and @mgartner)


pkg/sql/rowenc/testutils.go, line 1594 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

[nit] I think you could use RandDatum to get a random "special" datum for right, rather than hard-coding to 1.

Good idea, that also made the code simpler.

@RaduBerinde RaduBerinde force-pushed the sqlsmith-virtual-cols branch from 6a8b39a to 2809314 Compare March 4, 2021 21:17
@RaduBerinde
Copy link
Member Author

Had to make a few fixes: don't create FKs from computed columns or to virtual columns (which we don't support); and use FmtParsable in TestCreateRandomSchema.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)


pkg/sql/rowenc/testutils.go, line 1621 at r2 (raw file):

				},
				True: tree.NewStrVal("foo"),
				Else: tree.NewStrVal("bar"),

[nit] you could use RandDatum here too

This changes the random table generator to also create computed
columns (either STORED or VIRTUAL). Some example of definitions:
 - `col1_14 STRING NOT NULL AS (lower(CAST(col1_8 AS STRING))) VIRTUAL`
 - `col1_10 FLOAT8 AS (col1_5 + (-0.16607712222551002):::FLOAT8) STORED`
 - `col1_13 INT4 AS (col1_0 + col1_10) STORED`

Release justification: non-production code change.

Release note: None
Fixing this test to use FmtParsable. Without it, an expression that
contains an `Infinity` decimal isn't quoted properly.

Release justification: non-production code change

Release note: None
@RaduBerinde RaduBerinde force-pushed the sqlsmith-virtual-cols branch from 2809314 to d9949a3 Compare March 5, 2021 02:52
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

TFTR!

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


pkg/sql/rowenc/testutils.go, line 1621 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

[nit] you could use RandDatum here too

Done.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis)

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 5, 2021

Build succeeded:

@craig craig bot merged commit 2264789 into cockroachdb:master Mar 5, 2021
@RaduBerinde RaduBerinde deleted the sqlsmith-virtual-cols branch March 8, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants