-
Notifications
You must be signed in to change notification settings - Fork 239
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
[Datastore] Add Snowflake source #1845
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.
Pretty much straight forward, just 2 minor comments.
# V3IO does not support this level of precision | ||
df = df.withColumn(col_name, funcs.col(col_name).cast("double")) | ||
return df | ||
|
||
def write_dataframe( | ||
self, df, key_column=None, timestamp_key=None, chunk_id=0, **kwargs | ||
): | ||
if hasattr(df, "rdd"): |
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.
I guess "rdd" somehow means that it is spark engine but maybe we should add a comment so it would be more straight forward
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.
Yeah, that's what it means... It's done in a few other places in the code base and is not affected by this PR. Feel free to PR separately.
def __init__( | ||
self, | ||
name: str = "", | ||
attributes: Dict[str, str] = None, | ||
key_field: str = None, | ||
time_field: str = None, | ||
schedule: str = None, |
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.
It seems like you're not accepting all of the base class args - path
, start_time
and end_time
are missing, is that on purpose ?
schedule=schedule, | ||
) | ||
|
||
def get_spark_options(self): |
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 add an option "application": "Iguazio"
which will indicate the client is from Iguazio? (for the partnership @marcelonyc knows more details about this)
ML-1741