Loosen validation and mypy type requirements for date/time types, start increased testing #144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vinceatbluelabs
commented
Dec 9, 2020
| 'YYYY-MM-DD HH:MI:SS']: | ||
| pass | ||
| else: | ||
| cant_handle_hint(fail_if_cant_handle_hint, 'datetimeformattz', hints) | ||
| quiet_remove(unhandled_hints, 'datetimeformattz') | ||
|
|
||
| if hints.timeonlyformat == 'HH24:MI:SS': | ||
| if hints.timeonlyformat in ['HH24:MI:SS', 'HH:MI:SS']: |
There was a problem hiding this comment.
This fixes issues found during testing!
There was a problem hiding this comment.
Specifically, the Redshift spec for date/time formats defines HH as equivalent to HH24 - so there's no reason now to handle it similarly to the HH24 cases. These were reproduced with test cases defined in datetime_cases.py in tests/component below.
vinceatbluelabs
commented
Dec 9, 2020
| "YYYY-MM-DD HH:MI:SS", | ||
| "YYYY-MM-DD HH12:MI AM", | ||
| "MM/DD/YY HH24:MI", | ||
| ] |
There was a problem hiding this comment.
Not all of this file is probably used in this PR - it's the full set of things needed to define the test cases for #140
vinceatbluelabs
commented
Dec 9, 2020
| if should_raise[dateformat]: | ||
| pass | ||
| else: | ||
| raise |
There was a problem hiding this comment.
The general intent with these tests is that as we add new things to the below datetime_cases.py module, tests will blow up and we'll need to go back and hand-add expectations of what each driver should produce for each input.
vinceatbluelabs
commented
Dec 9, 2020
| @@ -324,13 +324,14 @@ def add_load_job_csv_config(unhandled_hints: Set[str], | |||
| quiet_remove(unhandled_hints, 'datetimeformat') | |||
|
|
|||
| if hints.datetimeformattz in ['YYYY-MM-DD HH:MI:SSOF', | |||
| 'YYYY-MM-DD HH24:MI:SSOF', | |||
| 'YYYY-MM-DD HH:MI:SS']: | |||
There was a problem hiding this comment.
YYYY-MM-DD HH24:MI:SS would be a good additional item - I'll write this down and address it in #140 if not before, probably by adding it as an explicit test case in my list of test cases below.
|
Looks like I have a test failure, which is annoying. I’ll open up another PR related to that and probably merge master back into this branch afterwards. If you could review the PR in the meantime ignoring the test failure for the moment, I’ll hold off merging until that fixes the passing tests. |
crvena-sonja
approved these changes
Dec 9, 2020
…om:bluelabsio/records-mover into loosen_date_requirements_1
This was referenced Dec 9, 2020
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 9, 2020
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Vertica databases. Along the way some places for improvement were found!
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 9, 2020
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Pandas. Along the way some places for improvement were found!
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 9, 2020
…147) * Improve date/time hint handling for Vertica based on better testing Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Vertica databases. Along the way some places for improvement were found! * Ratchet coverage * Ratchet mypy * Ratchet coverage * Ratchet mypy coverage
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 9, 2020
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Redshift databases. Along the way some places for improvement were found!
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 9, 2020
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Redshift databases. Along the way some places for improvement were found!
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 10, 2020
Based on the new testing approach in #144, this PR adds component tests for date/time types while loading CSV files in MySQL. The expectations for those test are based on the results of the integration tests in #140, which shows our MySQL driver is way too optimistic about what date/time hints it can import!
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 10, 2020
) * Improve date/time hint handling for Pandas based on better testing Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Pandas. Along the way some places for improvement were found! * Ratchet test coverage * Ratchet mypy coverage * Update records_mover/records/pandas/to_csv_options.py Co-authored-by: Chris Wegrzyn <[email protected]> Co-authored-by: Chris Wegrzyn <[email protected]>
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 10, 2020
* Improve date/time hint handling for MySQL based on better testing Based on the new testing approach in #144, this PR adds component tests for date/time types while loading CSV files in MySQL. The expectations for those test are based on the results of the integration tests in #140, which shows our MySQL driver is way too optimistic about what date/time hints it can import! * Make comment in complete sentences * Quick flake8 fix * csv variant proved to not be possible * Ratchet * Ratchet
vinceatbluelabs
added a commit
that referenced
this pull request
Dec 10, 2020
…149) * Improve date/time hint handling for Redshift based on better testing Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Redshift databases. Along the way some places for improvement were found! * Improve date/time hint handling for Redshift based on better testing Based on the new testing approach in #144, this PR adds component tests for date/time types while loading/exporting CSV files in Redshift databases. Along the way some places for improvement were found! * Ratchet coverage * Remove original * Ratchet
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Records Spec uses hints to describe how dates, times, datetime, and datetimetz types are formatted in CSVs.
It does this by describing a set of allowed values. The format of the values were originally cribbed from Redshift's COPY statement.
The current set of actually allowed values for those types (dateformat/timeonlyformat/datetimeformattz/datetimeformat) are fairly arbitrary, incomplete, inconsistent with each other, and don't hold up as terribly consistent under scrutiny. They've also been a source of bugs over time.
To make those complete even insomuch as the patterns they currently try to support would involve the start of some combinatorial explosion:
I think this does not bode well for maintaining an allowlist of formats - instead, I think we're going to need to (like Redshift) support a general formatting language in the spec, and do translations as needed to the formatting languages of the different databases/tools that Records Mover integrates with.
In the face of that, I'd like to expand the allowed values in a future PR. The motivation there is to ensure we allow likely DMY-flavored formats, to more generally support more things users may need in the future.
This PR is step one of that. It adds one additional test as a beginning of comprehensive testing of the existing supported values, and makes it easy to add more testable values in the future.
To deal with the combinatorial explosion problem for the time being, I've loosened the internal constraints by changing the Literal types to strings, and fixed up the various things based on what the testing found.
This PR does not yet loosen up the spec in any way.
The rest of the tests can be found in #140, which is probably too big to review right now--this is the first PR chipped off of that boulder.