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

sql/sqlbase: add a hard coded list of interesting datums to RandDatum #36964

Merged
merged 4 commits into from May 15, 2019

Conversation

Projects
None yet
5 participants
@mjibson
Copy link
Member

commented Apr 19, 2019

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

@mjibson mjibson requested a review from justinj Apr 19, 2019

@mjibson mjibson requested review from cockroachdb/cli-prs as code owners Apr 19, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

This change is Reviewable

@mjibson

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

First two commits from another PR, ignore them. Just needed for the constants from the new date package.

@andy-kimball

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Matt, I made significant changes to the random type/datum generation routines in the type system refactor PR. I'm getting ready to merge that. Can you rebase this on top of that?

@mjibson

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Yes, will do.

@mjibson mjibson force-pushed the mjibson:specials branch from 075943c to 0eb698d May 3, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

RFAL.

@mjibson mjibson requested a review from jordanlewis May 3, 2019

@mjibson mjibson force-pushed the mjibson:specials branch from 0eb698d to 9c21608 May 3, 2019

@jordanlewis
Copy link
Member

left a comment

:lgtm: nice idea!

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

@mjibson mjibson force-pushed the mjibson:specials branch from 9c21608 to f8fdb4c May 3, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 3, 2019

Merge #36964
36964: sql/sqlbase: add a hard coded list of interesting datums to RandDatum r=mjibson a=mjibson

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 3, 2019

Build failed

@mjibson mjibson force-pushed the mjibson:specials branch from f8fdb4c to e982886 May 6, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Added some commits to address failures caught by these improved tests! PTAL.

@mjibson mjibson force-pushed the mjibson:specials branch 2 times, most recently from 903d1d6 to 8685de4 May 6, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@mjibson it looks like there might still be some CI issues?

@jordanlewis
Copy link
Member

left a comment

:lgtm: though

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


pkg/sql/distsqlplan/aggregator_funcs_test.go, line 382 at r3 (raw file):

// https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
func almostEqualRelative(a, b float64) bool {

minor nit: I would consider moving some of the documentation above to this function's godoc.

@mjibson mjibson force-pushed the mjibson:specials branch from 8685de4 to f165b22 May 10, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 10, 2019

Merge #36964
36964: sql/sqlbase: add a hard coded list of interesting datums to RandDatum r=mjibson a=mjibson

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 10, 2019

Build failed

mjibson added some commits May 2, 2019

sql/sqlbase: add a hard coded list of interesting datums to RandDatum
This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None
sql: return NaN instead of an error on invalid decimal operations
Release note (sql change): Return NaN instead of an error during invalid
decimal operations.
sql/distsqlplan: fix TestDistAggregationTable tests
Teach TestDistAggregationTable to properly serialize datums so that Inf
and Nan aren't printed as raw strings.

Make the float comparator better and have it ignore NaNs because those
happen sometimes now.

Release note: None

@mjibson mjibson force-pushed the mjibson:specials branch from f165b22 to a4ccd33 May 13, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@danhhz Can you review the changefeed commit?

@danhhz
Copy link
Contributor

left a comment

changefeedccl :lgtm:

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


pkg/ccl/changefeedccl/avro_test.go, line 185 at r7 (raw file):

			// timestamps that don't fit in an int32, so
			// restrict input to fit.
			t := time.Unix(rng.Int63n(2e6)-1e6, rng.Int63n(1e9)).Truncate(time.Millisecond)

can you comment where all these constants come from? i actually don't know offhand

also nit: instead of rng.Int63n(2e6)-1e6, throw a small randInt(rng *rand.Rand, min, max int) int method in this file and use it

ccl/changefeedccl: limit date and timestamp tests to working data
These are existing bugs in goavro, so we can't fix the actual cause yet.

Release note: None

@mjibson mjibson force-pushed the mjibson:specials branch from 6d7fc91 to 0df1bce May 14, 2019

@mjibson
Copy link
Member Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @danhhz, @justinj, and @mjibson)


pkg/ccl/changefeedccl/avro_test.go, line 185 at r7 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

can you comment where all these constants come from? i actually don't know offhand

also nit: instead of rng.Int63n(2e6)-1e6, throw a small randInt(rng *rand.Rand, min, max int) int method in this file and use it

I rejiggered this and may have found the actual goavro bug, which is that time.Times you send to goavro must have a nanosecond representation that fits in an int64. The current code makes this clear.

@danhhz
Copy link
Contributor

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @justinj and @mjibson)


pkg/ccl/changefeedccl/avro_test.go, line 185 at r7 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I rejiggered this and may have found the actual goavro bug, which is that time.Times you send to goavro must have a nanosecond representation that fits in an int64. The current code makes this clear.

This looks great, thanks!

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 14, 2019

Merge #36964
36964: sql/sqlbase: add a hard coded list of interesting datums to RandDatum r=mjibson a=mjibson

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 14, 2019

Build failed

@mjibson mjibson force-pushed the mjibson:specials branch from 0df1bce to 000ec6e May 15, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 15, 2019

Merge #36964
36964: sql/sqlbase: add a hard coded list of interesting datums to RandDatum r=mjibson a=mjibson

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 15, 2019

Build failed

@mjibson mjibson force-pushed the mjibson:specials branch from 000ec6e to 0df1bce May 15, 2019

@mjibson

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 15, 2019

Merge #36964
36964: sql/sqlbase: add a hard coded list of interesting datums to RandDatum r=mjibson a=mjibson

This should improve randomized test coverage by producing more interesting
datums more often.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 15, 2019

Build succeeded

@craig craig bot merged commit 0df1bce into cockroachdb:master May 15, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@mjibson mjibson deleted the mjibson:specials branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.