-
Notifications
You must be signed in to change notification settings - Fork 6
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: create db_dtypes JSONDtype and JSONArray #284
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.
Not quite finished with the review, but sending comments now since I have a meeting and want to share what I have so far before I get back to it.
db_dtypes/json.py
Outdated
@property | ||
def type(self) -> type[str]: | ||
"""Return the scalar type for the array, e.g. int.""" | ||
return dict |
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.
Could this be a Union[dict, list, str, int, float]
?
Please create a JSONScalarType = Union[dict, list, str, int, float]
module-level variable and use it here, if so.
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 tried JSONScalarType
and typing.Any
but both of them are hitting errors on pandas source codes. Also, I didn't find an example Extension dtype that has multiple types. For now, I would leave it as a str to indicate its storage type and the type of to_numpy
as well. Here are the call stack of both of them:
https://screenshot.googleplex.com/6GiKkNU9T2e8GPm
https://screenshot.googleplex.com/9QfDFsrUzD7PMrT
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.
Another round of comments before I go to lunch. Thanks for your patience with the review.
5b8f379
to
9e92dc7
Compare
cebde95
to
efe72cc
Compare
865f93b
to
790f257
Compare
98debff
to
b4cfcd9
Compare
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.
LGTM once test coverage is reached.
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 internal bug b/312728178 🦕