-
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1872 +/- ##
==========================================
- Coverage 86.94% 86.93% -0.02%
==========================================
Files 59 59
Lines 5080 5104 +24
Branches 822 831 +9
==========================================
+ Hits 4417 4437 +20
- Misses 466 469 +3
- Partials 197 198 +1 ☔ 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 😁
with warnings.catch_warnings(record=True) as inspection_warnings: | ||
for column_def in inspected.get_columns(table_name, schema=schema_name): | ||
column_name = column_def["name"] | ||
is_nullable = column_def.get("nullable", False) | ||
jsonschema_type: dict = self.to_jsonschema_type( | ||
t.cast(sqlalchemy.types.TypeEngine, column_def["type"]), | ||
) | ||
table_schema.append( | ||
th.Property( | ||
name=column_name, | ||
wrapped=th.CustomType(jsonschema_type), | ||
required=not is_nullable, | ||
), | ||
) | ||
if len(inspection_warnings) > 0: | ||
for line in inspection_warnings: | ||
expanded_msg: str = ( |
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.
For this bits, wdyt about using the builtin logging.captureWarnings
?
We'd probably want to call it whenever a plugin's CLI is invoked, so in the body of
Lines 513 to 531 in 046044f
@classmethod | |
def invoke( | |
cls, | |
*, | |
about: bool = False, | |
about_format: str | None = None, | |
**kwargs: t.Any, # noqa: ARG003 | |
) -> None: | |
"""Invoke the plugin. | |
Args: | |
about: Display package metadata and settings. | |
about_format: Specify output style for `--about`. | |
kwargs: Plugin keyword arguments. | |
""" | |
if about: | |
cls.print_about(about_format) | |
sys.exit(0) | |
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.
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/