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
Support inspection of Redshift datatypes #242
Support inspection of Redshift datatypes #242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some early thoughts. Feel free to force push the content to the trigger-integration
branch of the main repo when you're ready to have integration tests run.
sqlalchemy_redshift/dialect.py
Outdated
@@ -260,6 +260,13 @@ def process_bind_param(self, value, dialect): | |||
return json.dumps(value) | |||
return value | |||
|
|||
# Mapping for database schema inspection of Amazon Redshift datatypes | |||
redshift_ischema_names = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define this constant in UPPER_CAMEL_CASE and move it to the top of the file with other constants?
I added a few more changes here as I noticed there were still some issues with type inspection
Use case: from sqlalchemy_redshift.dialect import TIMETZ, SUPER, TIMESTAMPTZ, GEOMETRY
for x in (TIMETZ(), TIMESTAMPTZ(), SUPER(), GEOMETRY()):
print(x) TIMETZ
TIMESTAMPTZ
SUPER
GEOMETRY previous behavior
I don’t think we can move |
Hmm, I pushed to |
I'll contact Travis support to get this unstuck.
Likewise! |
Thanks for getting Travis going again, @jklukas! Integration tests are all passing. Am I ok to merge? |
Looks good! Feel free to merge. |
This PR modifies
RedshiftDialectMixin
to support database schema inspection of Amazon Redshift datatypes. It adds entries inischema_names
for these Redshift specific datatypes. It also makes a small change to the definition ofTimetz
andTimestamptz
datatype definition for compatibility with SqlAlchemy insepctor.Use case: Support for Redshift datatypes in Querybook Metastore
Issue: When
inspect.get_columns(...)
is called,NullType()
is returned as the type for columns with typegeometry
andsuper
, which causes issues in downstream tools like Querybook's metastore.Repro:
where
my_table
is defined asFor columns with type
timetz
andtimestamptz
,sa.dialects.postgresql.TIME
andsa.dialects.postgresql.TIMESTAMP
are returned, which isn't ideal since we've definedTIMETZ
andTIMESTAMPTZ
datatypes withinsqlalchemy-redshift
.sqlalchemy.engine.dialects.postgresql.base.PGDialect._get_column_info
uses theischema_names
to map datatype string names to datatype classes.screenshots below show the behavior before & after this change when the above use case is run:
note entries 11, 12, 15, 16
behavior before:
behavior after:
Todos