-
Notifications
You must be signed in to change notification settings - Fork 10
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
Let pyarrow cast strings to dates #80
Conversation
"created": "2024-09-10T10:00:00.425293+00:00", | ||
"updated": "2024-09-10T10:00:00.425300+00:00", |
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.
only this first item has these properties set
@@ -1,6 +1,5 @@ | |||
"""Convert STAC data into Arrow tables""" | |||
|
|||
import ciso8601 |
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.
since pandas is a dependency, maybe ciso8601
could be dropped as a dependency and instead just use pandas or pyarrow for date handling? see also #31 (comment)
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.
Definitely don't want to use pandas here, but using pyarrow natively for the cast is fine.
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.
Looks like the only other place ciso8601
is used currently is in testing here
stac-geoparquet/tests/json_equals.py
Lines 107 to 108 in 4b00f5b
result_datetime = parse_rfc3339(result) | |
expected_datetime = parse_rfc3339(expected) |
Should I switch that to use pandas or pa.scalar(timestamp_str, type=pa.string()).cast(pa.timestamp('us', tz='UTC')).as_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.
I think it's fine for ciso8601 to still be used in the tests.
@@ -1,6 +1,5 @@ | |||
"""Convert STAC data into Arrow tables""" | |||
|
|||
import ciso8601 |
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.
Definitely don't want to use pandas here, but using pyarrow natively for the cast is fine.
Addresses #79