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.

Sign up for GitHub

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

Jump to bottom

sql: better support and testing for FmtExport #44322

Closed
maddyblue opened this issue Jan 23, 2020 · 6 comments
Closed

sql: better support and testing for FmtExport #44322

maddyblue opened this issue Jan 23, 2020 · 6 comments
Assignees
@rohany

Comments

@maddyblue
Copy link
Contributor

tree.FmtExport and ParseDatumStringAs are a pair of functions that are designed to round trip our datums as strings. They are tested at

cockroach/pkg/sql/sem/tree/parse_string_test.go

Line 26 in c16ce06

func TestParseDatumStringAs(t *testing.T) {
.

We need to improve support for them.

  1. FmtExport doesn't support arrays or tuples. It needs to be taught about them. ParseDatumStringAs must also be taught how to parse that format. Make sure recursive tuples and other weird things work.
  2. We should improve the testing to use the randInterestingDatums (

    cockroach/pkg/sql/sqlbase/testutils.go

    Line 364 in c16ce06

    randInterestingDatums = map[types.Family][]tree.Datum{
    ) which will verify that our edge case datums don't cause bugs.
  3. randInterestingDatums should be taught about all of our datums (it currently doesn't have UUIDs, arrays, and some other stuff).
  4. The randInterestingDatum func (

    cockroach/pkg/sql/sqlbase/testutils.go

    Line 527 in c16ce06

    func randInterestingDatum(rng *rand.Rand, typ *types.T) tree.Datum {
    ) should panic when a datum is requested that it can't generate. This will guarantee that anytime we add new types, we must also add their edge cases to this list.
  5. Maybe add arrays of arrays or tuples of tuples and other really weird stuff to the the rand datum functions (either randInterestingDatum or RandDatumWithNullChance).

Bonus points:

  • Make a test function that can generate a table with one column of each type, then insert all data from the randinterestingdatum map into that table.
  • Use this table to ensure that we can round trip the following things correctly:
    • cockroach dump / cockroach sql
    • BACKUP / RESTORE
    • EXPORT / IMPORT
    • CDC
    • distsql processor serialization
@rohany
Copy link
Contributor

rohany commented Jan 24, 2020

tree.FmtExport and ParseDatumStringAs are a pair of functions that are designed to round trip our datums as strings.

Is FmtExport a function or just a format flag for the FmtCtx? I'm only finding the former.

@maddyblue
Copy link
Contributor Author

Just a flag.

@rohany rohany mentioned this issue Jan 26, 2020
Closed
rohany added a commit to rohany/cockroach that referenced this issue Jan 26, 2020
@rohany
sqlbase: add randomized testing of ParseDatumStringAs
44b9fe9 
Part of cockroachdb#44322.

To ensure better testing of the roundtripping of datums
through `FmtExport` and `ParseDatumStringAs` this PR
adds another test that runs random datums through this path.

Release note: None
@rohany rohany mentioned this issue Jan 26, 2020
Merged
@rohany
Copy link
Contributor

rohany commented Jan 26, 2020

@mjibson, why do we need to support FmtExport / parsing of tuples? You wouldn't be able to like store a tuple in a column or anything, so why is that applicable here?

rohany added a commit to rohany/cockroach that referenced this issue Jan 27, 2020
@rohany
tree: add FmtExport formatting for array types
2242cb0 
Part of work for cockroachdb#44322.

Fixes cockroachdb#33429.

Release note (enterprise change): This PR fixes a bug where
export/import could not round trip arrays correctly.
@rohany
Copy link
Contributor

rohany commented Jan 27, 2020

I'm looking at this more, and I'm concerned if FmtExport is really the correct combination of flags for roundtripping strings --

For example, ARRAY[NULL, "NULL"] formats to ARRAY[NULL,NULL], which is incorrect. Or, ARRAY['"hello'] formats to ARRAY["hello] which doesn't parse.

Most of the various escaping done in TestParseArray does not seem to preserve some of the escaping in the input.

@rohany rohany mentioned this issue Jan 27, 2020
Closed
rohany added a commit to rohany/cockroach that referenced this issue Jan 27, 2020
@rohany
tree: add FmtExport formatting for array types
73525c5 
Part of work for cockroachdb#44322.

Fixes cockroachdb#33429.

Release note (enterprise change): This PR fixes a bug where
export/import could not round trip arrays correctly.
@rohany
Copy link
Contributor

rohany commented Jan 27, 2020

TODO (rohany): add a test that tests round tripping of arrays of all scalar types, or update the existing test to do so.

rohany added a commit to rohany/cockroach that referenced this issue Jan 27, 2020
@rohany
sqlbase: add randomized testing of ParseDatumStringAs
950819c 
Part of cockroachdb#44322.

To ensure better testing of the roundtripping of datums
through `FmtExport` and `ParseDatumStringAs` this PR
adds another test that runs random datums through this path.

Release note: None
craig bot pushed a commit that referenced this issue Jan 28, 2020
@rohany
Merge #44390
85df0c2 
44390: sqlbase: add randomized testing of ParseDatumStringAs r=mjibson a=rohany

Part of #44322.

To ensure better testing of the roundtripping of datums
through `FmtExport` and `ParseDatumStringAs` this PR
adds another test that runs random datums through this path.

Release note: None

Co-authored-by: Rohan Yadav <[email protected]>
rohany added a commit to rohany/cockroach that referenced this issue Jan 28, 2020
@rohany
sqlbase: add interesting datums for all scalar types
2d36a35 
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 rohany mentioned this issue Jan 28, 2020
Merged
@rohany rohany mentioned this issue Jan 28, 2020
Merged
rohany added a commit to rohany/cockroach that referenced this issue Jan 28, 2020
@rohany
sqlbase: add interesting datums for all scalar types
6db2dd4 
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 added a commit to rohany/cockroach that referenced this issue Jan 29, 2020
@rohany
sqlbase: add round-trippable export and parsing for arrays
99469eb 
Work for cockroachdb#44322.

This PR adds support for ARRAY's to be dumped with the
`tree.FmtExport` flag, which dumps datums in a string
format that can be round-tripped with `sqlbase.ParseDatumStringAs`.

Release note: None
@rohany rohany mentioned this issue Jan 30, 2020
Merged
rohany added a commit to rohany/cockroach that referenced this issue Jan 30, 2020
@rohany
sqlbase: add table generator for edge case datums
ab08e1f 
Part of work for cockroachdb#44322.

This PR adds a testing utility to generate a table
full of edge case datums to test a variety of things in the future.

Release note: None
rohany added a commit to rohany/cockroach that referenced this issue Jan 30, 2020
@rohany
sqlbase: add interesting datums for all scalar types
1804738 
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 added a commit to rohany/cockroach that referenced this issue Jan 30, 2020
@rohany
sqlbase: add table generator for edge case datums
5513452 
Part of work for cockroachdb#44322.

This PR adds a testing utility to generate a table
full of edge case datums to test a variety of things in the future.

Release note: None
craig bot pushed a commit that referenced this issue Jan 30, 2020
@rohany
Merge #44459
555103f 
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 bot pushed a commit that referenced this issue Jan 30, 2020
@koorosh @rohany
Merge #44078 #44545
6bf3a7b 
44078: ui: Tests on react-routing components r=dhartunian a=Koorosh

Depends on PR: #43846
`yarn-vendor` PR has to be merged: cockroachdb/yarn-vendored#15

Current PR includes minimal changes to functionality (only those which are required to isolate or get
access to individual components under tests):
- `src/index.tsx` file has been refactored into two parts: rendering to DOM and independent `App` component;
-  <Helmet> component (`react-helmet` library) has been refactored to avoid known issue during tests execution (nfl/react-helmet#373)
- old version of react-router and `connect` component in tandem do not work well because connected
component aren't valid react components. As result validation for correct routing was done by re-exporting wrapped components (instead of connected). It allowed to simplify testing with `enzyme#find` function which requires valid react component as an argument;
- Tests are grouped in suites per route to maintain related tests per route;
- There is two kinds of tests which test correct rendering of component for
specific test and another test which validates correct redirection for routes;
- Tests can be refactored to be constructed from configuration to reduce repeated
tests, but it will be actual after migrating to latest version of react-router lib.

44545: sqlbase: add table generator for edge case datums r=rohany a=rohany

Part of work for #44322.

This PR adds a testing utility to generate a table
full of edge case datums to test a variety of things in the future.

Release note: None

Co-authored-by: Andrii Vorobiov <[email protected]>
Co-authored-by: Rohan Yadav <[email protected]>
rohany added a commit to rohany/cockroach that referenced this issue Feb 4, 2020
@rohany
sqlbase: add round-trippable export and parsing for arrays
3ca040f 
Work for cockroachdb#44322.

This PR adds support for ARRAY's to be dumped with the
`tree.FmtExport` flag, which dumps datums in a string
format that can be round-tripped with `sqlbase.ParseDatumStringAs`.

This PR additionally performs some code cleanup and renaming
around various parsing functions within the `tree` package.

Release note: None
rohany added a commit to rohany/cockroach that referenced this issue Feb 4, 2020
@rohany
sqlbase: add round-trippable export and parsing for arrays
899796b 
Work for cockroachdb#44322.

This PR adds support for ARRAY's to be dumped with the
`tree.FmtExport` flag, which dumps datums in a string
format that can be round-tripped with `sqlbase.ParseDatumStringAs`.

This PR additionally performs some code cleanup and renaming
around various parsing functions within the `tree` package.

Release note: None
@rohany rohany mentioned this issue Feb 5, 2020
Closed
rohany added a commit to rohany/cockroach that referenced this issue Feb 5, 2020
@rohany
sqlbase: add round-trippable export and parsing for arrays and collat…
dbfd995 
…ed strings

Work for cockroachdb#44322.
Fixes cockroachdb#33429.
Fixes cockroachdb#44414.
Closes cockroachdb#44669.

This PR adds support for ARRAY's and collated strings to be dumped with the
`tree.FmtExport` flag, which dumps datums in a string format that can be
round-tripped with `sqlbase.ParseDatumStringAs`.

This PR additionally performs some code cleanup and renaming
around various parsing functions within the `tree` package.
In particular, this PR removes the function `tree.ParseStringAs` which
was confusingly used sometimes instead of `sqlbase.ParseDatumStringAs`
due to it's handling of raw bytes. Instead, we introduce a new function
`sqlbase.ParseDatumStringAsWithRawBytes` to be more explicit about when
to use one parsing function over the other.

Release note (bug fix): This PR fixes bugs around `cockroach dump` and
`IMPORT`/`EXPORT` where columns of arrays or collated strings were not able
to be roundtripped from cockroach to the dump and back to cockroach.
craig bot pushed a commit that referenced this issue Feb 5, 2020
@rohany
Merge #44464
2859ad3 
44464: sqlbase: add round-trippable export and parsing for arrays and collated strings r=knz a=rohany

Work for #44322.
Fixes #33429.
Fixes #44414.
Closes #44669.

This PR adds support for ARRAY's and collated strings to be dumped with the
`tree.FmtExport` flag, which dumps datums in a string format that can be
round-tripped with `sqlbase.ParseDatumStringAs`.

This PR additionally performs some code cleanup and renaming
around various parsing functions within the `tree` package.
In particular, this PR removes the function `tree.ParseStringAs` which
was confusingly used sometimes instead of `sqlbase.ParseDatumStringAs`
due to it's handling of raw bytes. Instead, we introduce a new function
`sqlbase.ParseDatumStringAsWithRawBytes` to be more explicit about when
to use one parsing function over the other.

Release note (bug fix): This PR fixes bugs around `cockroach dump` and
`IMPORT`/`EXPORT` where columns of arrays or collated strings would
not be able to be roundtripped from cockroach to the dump and back to cockroach.

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

rohany commented Feb 6, 2020

This has been completed, with extra testing moved to a new issue.

@rohany rohany closed this as completed Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rohany rohany

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@maddyblue @rohany