Added support for tinyint to _parse.py #315
Conversation
TimTheinAtTabs
requested review from
susodapop,
arikfr,
yunbodeng-db and
andrefurlan-db
as code owners
|
Please push an update to your commit including the DCO sign-off as described here. |
There was a problem hiding this comment.
Sorry for the chatter. I clicked the approve button prematurely. One comment for you:
Before we can merge:
- We need to add e2e / unit tests for this. I can do this for you if you're not able to do so easily.
- We can't merge this change without a DCO sign-off on your commit. Please amend your commit and push it to your branch.
src/databricks/sqlalchemy/_parse.py
Outdated
| @@ -282,6 +282,7 @@ def get_pk_strings_from_dte_output( | |||
| GET_COLUMNS_TYPE_MAP = { | |||
| "boolean": sqlalchemy.types.Boolean, | |||
| "smallint": sqlalchemy.types.SmallInteger, | |||
| "tinyint": sqlalchemy.types.SmallInteger, | |||
There was a problem hiding this comment.
This should be type_overrides.TINYINT, not SmallInteger, no?
There was a problem hiding this comment.
I do not know enough of the code to make the judgement call. I used my best guess to figure out the quickest solution that could work.
There was a problem hiding this comment.
Understood. Databricks has both SMALLINT and TINYINT types. The code you're editing has to do with column reflection of existing tables. These are mapped to SqlAlchemy types named SmallInteger (which is built-in to SQLAlchemy) and TINYINT (which is a custom type implemented in our dialect). In Python, a TINYINT in our dialect behaves exactly the same as a SmallInteger (Python doesn't have any size restrictions for integers). That's why your version of the code works for your use case. But we should reflect the type properly back as `type_overrides.TINYINT. Otherwise SQLAlchemy will not properly reflect back the true schema of the table, which will be an issue for people using SQLAlchemy/Alembic to programmatically manage their table schemas.
and fix pytest warning around returning an assertion Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
|
I've pushed a fix that properly tests this behaviour. Can you update your commit with the DCO sign-off? |
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
| @@ -111,7 +111,7 @@ def test_numeric_renders_as_decimal_with_precision(self): | |||
| ) | |||
|
|
|||
| def test_numeric_renders_as_decimal_with_precision_and_scale(self): | |||
| return self._assert_compiled_value_explicit( | |||
| self._assert_compiled_value_explicit( | |||
There was a problem hiding this comment.
I found during testing that this test case would always pass because prefixing an assertion with return always evaluates as a pass. When I removed the return, the test failed for TINYINT types (should have been caught during initial development). This test now passes.
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Using SqlAlchemy to reflect on a table having a tinyint column throws an error because tinyint type was missing from the type map. Adding a single line to the code seems to have fixed the bug.