-
Notifications
You must be signed in to change notification settings - Fork 163
Support inspection of Redshift datatypes #242
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
Conversation
jklukas
left a comment
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
| 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
GEOMETRYprevious 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
RedshiftDialectMixinto support database schema inspection of Amazon Redshift datatypes. It adds entries inischema_namesfor these Redshift specific datatypes. It also makes a small change to the definition ofTimetzandTimestamptzdatatype 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 typegeometryandsuper, which causes issues in downstream tools like Querybook's metastore.Repro:
where
my_tableis defined asFor columns with type
timetzandtimestamptz,sa.dialects.postgresql.TIMEandsa.dialects.postgresql.TIMESTAMPare returned, which isn't ideal since we've definedTIMETZandTIMESTAMPTZdatatypes withinsqlalchemy-redshift.sqlalchemy.engine.dialects.postgresql.base.PGDialect._get_column_infouses theischema_namesto 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