-
Notifications
You must be signed in to change notification settings - Fork 203
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: support datetime objects in literal instantiation #1542
feat: support datetime objects in literal instantiation #1542
Conversation
Thanks for the PR! Lets add a test for the example in the description with |
Added, let me know if we want anything more explicit there |
tests/integration/test_reads.py
Outdated
def test_scan_with_datetime(catalog: Catalog) -> None: | ||
table = create_table(catalog) | ||
# test that this doesn't raise an exception... | ||
table.scan(row_filter=GreaterThanOrEqual("datetime", datetime.now())).to_pandas() |
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.
nit: the table has datetime
column, what if we add a row and then filter datetime before and after and assert the results.
maybe something like,
yesterday = ...
table.append({"datatime": yesterday)
assert len(table.scan(row_filter=GreaterThanOrEqual("datetime", datetime.now()))) == 0
assert len(table.scan(row_filter=LessThanOrEqual("datetime", datetime.now()))) == 1
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.
thanks, looking much better now, took me too long to figure out I can run make test-integration PYTEST_ARGS='-k test_scan_with_datetime'
to only select that test hahaha
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! Thanks again
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 good, thanks for adding this @jayceslesar 🙌 Thanks for the review @kevinjqliu 🚀
pyiceberg/expressions/literals.py
Outdated
@@ -145,6 +147,8 @@ def literal(value: L) -> Literal[L]: | |||
return BinaryLiteral(value) | |||
elif isinstance(value, Decimal): | |||
return DecimalLiteral(value) | |||
elif isinstance(value, datetime): | |||
return TimestampLiteral(datetime_to_micros(value)) |
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.
The tzinfo
is handled here:
iceberg-python/pyiceberg/utils/datetime.py
Lines 75 to 81 in 3981e62
def datetime_to_micros(dt: datetime) -> int: | |
"""Convert a datetime to microseconds from 1970-01-01T00:00:00.000000.""" | |
if dt.tzinfo: | |
delta = dt - EPOCH_TIMESTAMPTZ | |
else: | |
delta = dt - EPOCH_TIMESTAMP | |
return (delta.days * 86400 + delta.seconds) * 1_000_000 + delta.microseconds |
this introduces sort of a tricky typing problem...
Will check it out tonight but can't just fix by adding |
Thanks for the contribution @jayceslesar! and thanks for the review @Fokko :) |
Support
datetime
objects being passed into theliteral
function.Allows situations like the following:
Which previously failed
Closes #1456