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

sqlbase: add interesting datums for all scalar types #44459

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Jan 28, 2020

This PR extends randInterestingDatums to include at least
one interesting datum for all scalar types to ensure that
edge cases don't break things. Additionally, this PR adds
a panic to randInterestingDatums if an interesting datum
for a type could not be found, so that we continue to have
coverage for all of our types.

Part of work for #44322.

Release note: None

@rohany rohany requested a review from yuzefovich January 28, 2020 21:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich 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 2 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @yuzefovich)


pkg/sql/sem/tree/datum.go, line 1485 at r1 (raw file):

}

var DMinUUID = NewDUuid(DUuid{uuid.UUID{}})

These will probably need comments (to satisfy the linter).


pkg/sql/sqlbase/testutils.go, line 579 at r1 (raw file):

	specials, ok := randInterestingDatums[typ.Family()]
	if !ok || len(specials) == 0 {
		panic(fmt.Sprintf("no interesting datum for type %s found", typ.String()))

Not all type families are present in the generation, so I think it's not valid to panic here. You could check whether typ is one of scalar types and then panic, or add all families to "interesting families", or not panic at all.

I guess I'd vote for not panicking here.

@rohany
Copy link
Contributor Author

rohany commented Jan 28, 2020

I think @mjibson wanted to panic here, but maybe checking if it is a scalar type and then panicking is good first step.

@rohany rohany force-pushed the better-random branch 2 times, most recently from a28755d to 6db2dd4 Compare January 28, 2020 22:33
Copy link
Member

@yuzefovich yuzefovich 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.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany)

@rohany
Copy link
Contributor Author

rohany commented Jan 29, 2020

This seems to have uncovered a bug around TimeTZ! I talked to @otan about it and we'll wait on that fix before merging.

@otan
Copy link
Contributor

otan commented Jan 30, 2020

fwiw, time is broken too. the problem is the test (and through auditing, cli/dump.go) will parse the special 24 hour time 24:00:00 as 00:00:00. The reason why the test does NOT hit this is because dTimeMax is currently set at 1 microsecond BEFORE 24:00:00.

Filed #44530, will get to it next week.

This PR extends randInterestingDatums to include at least
one interesting datum for all scalar types to ensure that
edge cases don't break things. Additionally, this PR adds
a panic to randInterestingDatums if an interesting datum
for a type could not be found, so that we continue to have
coverage for all of our types.

Part of work for cockroachdb#44322.

Release note: None
@rohany
Copy link
Contributor Author

rohany commented Jan 30, 2020

bors r+

craig bot pushed a commit that referenced this pull request Jan 30, 2020
44459: sqlbase: add interesting datums for all scalar types r=rohany a=rohany

This PR extends randInterestingDatums to include at least
one interesting datum for all scalar types to ensure that
edge cases don't break things. Additionally, this PR adds
a panic to randInterestingDatums if an interesting datum
for a type could not be found, so that we continue to have
coverage for all of our types.

Part of work for #44322.

Release note: None

Co-authored-by: Rohan Yadav <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jan 30, 2020

Build succeeded

@craig craig bot merged commit 1804738 into cockroachdb:master Jan 30, 2020
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.

4 participants