-
Notifications
You must be signed in to change notification settings - Fork 48
feat: Use validated local storage for data uploads #1612
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
Conversation
pandas_dataframe_copy.columns = pandas.Index(new_col_ids) | ||
pandas_dataframe_copy[ordering_col] = np.arange(pandas_dataframe_copy.shape[0]) | ||
|
||
timedelta_cols = utils.replace_timedeltas_with_micros(pandas_dataframe_copy) |
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.
you may wan to remove this function from utils.
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.
makes sense, yeah, done
bigframes/session/loader.py
Outdated
|
||
# JSON support incomplete | ||
for item in data.schema.items: | ||
utils.validate_dtype_can_load(item.column, item.dtype) |
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.
an we move valid_dtype_can_load
into loader.py
? It was put in utils.py
for referencing by two places.
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.
sure, done
bigframes/session/loader.py
Outdated
@@ -763,3 +740,39 @@ def _transform_read_gbq_configuration(configuration: Optional[dict]) -> dict: | |||
configuration["jobTimeoutMs"] = timeout_ms | |||
|
|||
return configuration | |||
|
|||
|
|||
def _search_for_nested_json_type(arrow_type: pa.DataType) -> bool: |
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.
naming nit: _has_json_arrow_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.
done
we're using a workaround: storing JSON as strings and then parsing them into JSON | ||
objects. | ||
TODO(b/395912450): Remove workaround solution once b/374784249 got resolved. | ||
""" |
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.
We should probably add some content in the Python doc to indicate that an error will be raised if the validation fails.
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.
added docstring raises
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕