SQLAlchemy 2: implement and refactor schema reflection methods #249
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
added 7 commits
October 13, 2023 19:14
5 skipped, 3 warnings, 727 errors in 40.01s Signed-off-by: Jesse Whitehouse <[email protected]>
It requires the implementation of raising NoSuchTableError for the different methods it checks. And hooboy was this difficult to track down! I found that these tests were mostly failing during setup and teardown because I didn't understand that two different schemas are required. This is covered in the SQLAlchemy tests documentation here: https://github.com/sqlalchemy/sqlalchemy/blob/main/README.unittests.rst#database-configuration The skipped tests happen because of exclusions, which is why the skip log information isn't as pleasant as our other tests. test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_check_constraints-_exclusions_06] SKIPPED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_columns-_exclusions_01] b'table_does_not_exists' PASSED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_foreign_keys-_exclusions_03] b'No such table table_does_not_exists' PASSED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_indexes-_exclusions_04] SKIPPED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_pk_constraint-_exclusions_02] b'No such table table_does_not_exists' PASSED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_table_comment-_exclusions_07] SKIPPED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_table_options-_exclusions_00] SKIPPED test_suite.py::ComponentReflectionTest_databricks+databricks::test_not_existing_table[get_unique_constraints-_exclusions_05] SKIPPED Signed-off-by: Jesse Whitehouse <[email protected]>
Signed-off-by: Jesse Whitehouse <[email protected]>
call to get_columns Signed-off-by: Jesse Whitehouse <[email protected]>
This passes the HasTableTest test group in the dialect compliance test suite Signed-off-by: Jesse Whitehouse <[email protected]>
This now PASSES test_get_primary_keys and test_get_pk_constraint Needs further refactor into actual parse methods... Signed-off-by: Jesse Whitehouse <[email protected]>
Signed-off-by: Jesse Whitehouse <[email protected]>
susodapop
requested review from
arikfr,
yunbodeng-db and
andrefurlan-db
as code owners
susodapop
commented
Oct 13, 2023
Comment on lines
+40
to
+41
| DBR_LTE_12_NOT_FOUND_STRING = "Table or view not found" | ||
| DBR_GT_12_NOT_FOUND_STRING = "TABLE_OR_VIEW_NOT_FOUND" |
There was a problem hiding this comment.
These duplicate lines will be removed in a subsequent PR.
benc-db
reviewed
Oct 13, 2023
| DBR_LTE_12_NOT_FOUND_STRING = "Table or view not found" | ||
| DBR_GT_12_NOT_FOUND_STRING = "TABLE_OR_VIEW_NOT_FOUND" | ||
|
|
||
|
|
There was a problem hiding this comment.
Why is all of this stuff in init.py?
There was a problem hiding this comment.
It's a byproduct of earlier decisions. I'll move it out of init.py in a follow-up PR.
There was a problem hiding this comment.
The guidance from sqlalchemy is that this should be in a file called base.py.
benc-db
approved these changes
Oct 13, 2023
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.
Background
The biggest piece of work required for this PR was figuring how to get
ComponentReflectionTestto even run as it's setUp and tearDown methods were failing for various reasons.Description
This PR changes the has_table, get_pk_constraint, and get_foreign_keys methods into functional shape. They now pass the related tests in ComponentReflectionTest.
get_foreign_keysis finished, following the guidance from @benc-db with regard to encapsulating strange parsing logic into more, smaller methods.In a follow-up PR I will make the same refactor to
get_pk_constraint, but I wanted to get eyes on the approach forget_foreign_keysbefore I proceed.I implemented a new
_describe_table_extendedmethod that is re-used by severaral methods.