-
Notifications
You must be signed in to change notification settings - Fork 8
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
add support to to_singer to build schema from catalog #42
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.
@keyn4 left some questions for you below
gluestick/singer.py
Outdated
@@ -225,13 +303,21 @@ def to_singer( | |||
Allow or not objects to the parsed, if false defaults types to str. | |||
|
|||
""" | |||
catalog_schema = os.environ.get("USE_CATALOG_SCHEMA", False) |
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.
Shouldn't this be os.environ.get("USE_CATALOG_SCHEMA", "false") == "true"
? Env vars are always strings
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.
fixed, thank you! 😃
@@ -196,6 +205,75 @@ def deep_convert_datetimes(value): | |||
return value.strftime("%Y-%m-%dT%H:%M:%S.%fZ") | |||
return value | |||
|
|||
def parse_objs(x): |
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.
How come you moved this here from etl_utils.py
?
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.
to avoid circular imports, to_export
here uses to_singer
from singer.py
and I wanted to use this function in singer.py
functions :c
import pandas as pd | ||
import pyarrow.parquet as pq | ||
|
||
class Reader: |
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.
Was this just a full copy paste moving it out of etl_utils
or did you change anything?
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.
yes full copy paste, no changes at all
Changes:
to_singer
to generate a schema using the catalog, it uses env varUSE_CATALOG_SCHEMA
Others: