-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Add sql-datatype to the SDK discovery and catalog #1872
base: main
Are you sure you want to change the base?
feat: Add sql-datatype to the SDK discovery and catalog #1872
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1872 +/- ##
==========================================
+ Coverage 90.88% 90.91% +0.02%
==========================================
Files 62 62
Lines 5165 5182 +17
Branches 667 671 +4
==========================================
+ Hits 4694 4711 +17
Misses 330 330
Partials 141 141 ☔ View full report in Codecov by Sentry. |
I have run across instances where the discovery process runs into SQL data types that SQLAlchemy cannot handle. When this occurs, the following warnings are logged to the console.
The
|
The warnings are being captured, the message expanded, and presented back to the console at the
|
After looking at this more I update the message to this format:
|
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.
Thanks @BuzzCutNorman! Left a few suggestions and opened #1903 which will make sense once taps output this type of metadata 😁
singer_sdk/connectors/sql.py
Outdated
for column_def in inspected.get_columns(table_name, schema=schema_name): | ||
sql_datatypes[ | ||
str(column_def["name"]) | ||
] = self.discover_catalog_entry_sql_datatype(column_def["type"]) |
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.
Would you feel comfortable adding tests for this? A good place might be in tests/core/test_connector_sql.py
since this is a connector method. I think it could even be parametrized with many examples of column types.
CodSpeed Performance ReportMerging #1872 will not alter performanceComparing Summary
|
553dec4
to
fc5ecc6
Compare
One thing that unfortunately isn't very clear to me is how are targets supposed to consume this information since a target isn't aware of the tap's metadata. It might be better if this metadata lived in the schema, at least that way it would be emitted with The official Singer docs only explain the field as https://github.com/search?q=org%3Atransferwise+sql-datatype+language%3APython&type=code&l=Python Otherwise, who is the consumer of this metadata? |
This is an attempt to add the sql_datatype to the discovery process and will be reflected in the catalog.
Closes #1323
📚 Documentation preview 📚: https://meltano-sdk--1872.org.readthedocs.build/en/1872/