You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sungwy (via GitHub)" <gi...@apache.org> on 2025/02/10 03:09:39 UTC
[PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
sungwy opened a new pull request, #1632:
URL: https://github.com/apache/iceberg-python/pull/1632
Fixes: https://github.com/apache/iceberg-python/issues/1552
The PR is still in progress.
- [x] Add TimestampNanoType and TimestampTzNanoType
- [x] Add Readers and Writers
- [ ] Update PyIceberg to use nanoseconds supported python date time representation
- [ ] Enhance Transforms
- [ ] Add String Expressions parsing for nanoseconds timestamps
- [x] Add format-version compatibility check for each type
- [ ] Run compatibility check on read and on write
- [ ] Unit tests
- [ ] Integration tests
python native `datetime` module does not have support for nanoseconds. We'll need to update our internal date time representations to use a different library. numpy? arrow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1977678422
##########
pyiceberg/types.py:
##########
@@ -175,6 +179,10 @@ def is_primitive(self) -> bool:
def is_struct(self) -> bool:
return isinstance(self, StructType)
+ def minimum_format_version(self) -> int:
Review Comment:
Let's throw in a nice alias:
```suggestion
def minimum_format_version(self) -> TableVersion:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2003796102
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
hm yea, now we have 2 `TableVersion`. one in `types.py`, the other in [`typedef.py`](https://github.com/apache/iceberg-python/blob/c06e32034fbd32d5aab9b2ef19ad2d253141479d/pyiceberg/typedef.py#L225)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2003191972
##########
pyiceberg/types.py:
##########
Review Comment:
I think we can do type promotions in a separate PR since there are a few edge cases with overflow that we need to take into account.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2003800525
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
this is only used in `minimum_format_version`. maybe we can use the `types.TableVersion` int literal for now and punt the enum discussion to #851
```
def minimum_format_version(self) -> TableVersion:
"""Minimum Iceberg format version after which this type is supported."""
return 1
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#issuecomment-2647514309
> python native datetime module does not have support for nanoseconds. We'll need to update our internal date time representations to use a different library. numpy? arrow?
Ideally, we want to use `int` as much as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2003187678
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
Should we move this next to the `TypeAlias`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002336680
##########
pyiceberg/types.py:
##########
Review Comment:
sure that sounds great! i just want to make sure we are tracking the type promotion feature for pyiceberg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#issuecomment-2741926678
> V3 Tracking issue #1818
I removed this integration test because we can't write V3 metadata yet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#issuecomment-2736444700
Looks like there is some issue with the `UnknownType`, we need to pass in the version there for the create table:
```
tbl = catalog.create_table(
identifier,
schema=arrow_table.schema,
+ properties={"format-version": "3"},
)
```
in `test_reads.py:1008`.
I think this PR is ready to go. There are some parts we need to double-check, for example, the downcasting of micros to nanos, and that we don't have to fail for V3 when that isn't set. But since we don't yet support producing V3 metadata, I think that's okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#issuecomment-2746373183
Thanks for working on this @sungwy, and thanks for the reviews @smaheshwar-pltr and @kevinjqliu 🙌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002306704
##########
pyiceberg/types.py:
##########
Review Comment:
Thanks for chiming in @smaheshwar-pltr . Appreciate the review.
I read the last column that mentioned that promotion to timestamp and timestamp_ns. is "not" allowed, but looking at the column for v3 spec, it seems to suggest otherwise. Hence I think this is worth clarifying with the community.
Referring to the Java code, I see that there is no valid promotion specified for `Date` type. Let me raise question in the dev list and clarify this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002211292
##########
pyiceberg/transforms.py:
##########
Review Comment:
Yes we should! Currently I'm getting the following exception from pyiceberg_core, and it looks like iceberg-rust's Bucket transform doesn't support nanosecond precision timestamps yet:
```
FeatureUnsupported => Unsupported data type for bucket transform: Timestamp(Nanosecond, Some("Etc/GMT+10"))
```
I've raised: https://github.com/apache/iceberg-rust/issues/1110 to track this issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002311502
##########
pyiceberg/types.py:
##########
Review Comment:
Thanks for chiming in @smaheshwar-pltr . Appreciate the review. I just read the spec again, and it the spec seems to suggest the following:
- date to timestamp and timestamp_ns is supported
- date to timestamptz and timestamptz_ns is "not" supported
Referring to the Java code, I see that there is no valid promotion specified for Date type. Would it make sense to introduce support for nanoseconds type first, and then work on introducing the reference implementation in Java first for this type promotion before implementing in PyIceberg?
@Fokko @kevinjqliu
##########
pyiceberg/types.py:
##########
Review Comment:
Thanks for chiming in @smaheshwar-pltr . Appreciate the review.
I read the last column that mentioned that promotion to timestamp and timestamp_ns. is "not" allowed, but looking at the column for v3 spec, it seems to suggest otherwise. Hence I think this is worth clarifying with the community.
Referring to the Java code, I see that there is no valid promotion specified for `Date` type. Let me raise question in the dev list and clarify this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#issuecomment-2677291293
I think it would make sense to break this PR up into two separate items:
1. one to introduce timestamp_ns and timestamptz_ns support
2. one to introduce parsing NS timestamps from string expressions with nano precision timestamp
This PR will focus on item (1)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002311502
##########
pyiceberg/types.py:
##########
Review Comment:
I just read it again, it the spec seems to suggest the following:
- date to timestamp and timestamp_ns is supported
- date to timestamptz and timestamptz_ns is "not" supported
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2006637432
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
Yeah I didn't know the `TypeAlias` existed - I've removed the `IntEnum` and used the `TypeAlias` instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "smaheshwar-pltr (via GitHub)" <gi...@apache.org>.
smaheshwar-pltr commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002210663
##########
pyiceberg/types.py:
##########
Review Comment:
Probably missing something but I read [here](https://iceberg.apache.org/spec/#schema-evolution) that `date` can be promoted to `timestamp_ns`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1977676297
##########
pyiceberg/transforms.py:
##########
@@ -323,6 +327,16 @@ def hash_func(v: Any) -> int:
return mmh3.hash(struct.pack("<q", v))
+ elif isinstance(source, (TimestampNanoType, TimestamptzNanoType)):
+
+ def hash_func(v: Any) -> int:
+ if isinstance(v, py_datetime.datetime):
+ v = datetime.datetime_to_micros(v)
Review Comment:
Haha - I meant to leave a comment here. Will do so we aren't confused reading this in the future
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1997259581
##########
pyiceberg/types.py:
##########
@@ -175,6 +179,10 @@ def is_primitive(self) -> bool:
def is_struct(self) -> bool:
return isinstance(self, StructType)
+ def minimum_format_version(self) -> int:
Review Comment:
Sounds good! Thanks for the suggestion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002196096
##########
pyiceberg/types.py:
##########
@@ -703,6 +718,44 @@ class TimestamptzType(PrimitiveType):
root: Literal["timestamptz"] = Field(default="timestamptz")
+class TimestampNanoType(PrimitiveType):
+ """A TimestampNano data type in Iceberg can be represented using an instance of this class.
+
+ TimestampNanos in Iceberg have nanosecond precision and include a date and a time of day without a timezone.
+
+ Example:
+ >>> column_foo = TimestampNanoType()
+ >>> isinstance(column_foo, TimestampNanoType)
+ True
+ >>> column_foo
+ TimestampNanoType()
+ """
+
+ root: Literal["timestamp_ns"] = Field(default="timestamp_ns")
+
+ def minimum_format_version(self) -> TableVersion:
+ return TableVersion.THREE
Review Comment:
great suggestion @kevinjqliu thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002195447
##########
pyiceberg/types.py:
##########
Review Comment:
I don't see any type promotions supported to and from `timestamp_ns` and `timestamptz_ns` types in the Spec:
https://iceberg.apache.org/spec/#primitive-types
And pulled up some issues that have been closed as unplanned:
- https://github.com/apache/iceberg/issues/9065
And discussion thread where the community decided to vote against introducing long -> timestamp_ns conversions:
https://lists.apache.org/thread/xd7v2znttzvvgx3sqbjf93zn8dwx0v7f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2009202742
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
No problem, let's defer that to #851
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1999548823
##########
pyiceberg/transforms.py:
##########
Review Comment:
should we include ns support in pyiceberg_core transform as well?
##########
pyiceberg/types.py:
##########
@@ -703,6 +718,44 @@ class TimestamptzType(PrimitiveType):
root: Literal["timestamptz"] = Field(default="timestamptz")
+class TimestampNanoType(PrimitiveType):
+ """A TimestampNano data type in Iceberg can be represented using an instance of this class.
+
+ TimestampNanos in Iceberg have nanosecond precision and include a date and a time of day without a timezone.
+
+ Example:
+ >>> column_foo = TimestampNanoType()
+ >>> isinstance(column_foo, TimestampNanoType)
+ True
+ >>> column_foo
+ TimestampNanoType()
+ """
+
+ root: Literal["timestamp_ns"] = Field(default="timestamp_ns")
+
+ def minimum_format_version(self) -> TableVersion:
+ return TableVersion.THREE
Review Comment:
UnknownType was just added in #1681 lets make sure this is properly set there too
##########
pyiceberg/types.py:
##########
Review Comment:
do we handle type promotion somewhere?
https://iceberg.apache.org/spec/#schema-evolution
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
interesting to see this, we were talking about something like this in #851
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1997260223
##########
pyiceberg/utils/datetime.py:
##########
@@ -91,6 +91,57 @@ def timestamp_to_micros(timestamp_str: str) -> int:
raise ValueError(f"Invalid timestamp without zone: {timestamp_str} (must be ISO-8601)")
+def time_str_to_nanos(time_str: str) -> int:
+ """Convert an ISO-8601 formatted time to nanoseconds from midnight."""
+ return time_to_nanos(time.fromisoformat(time_str))
+
+
+def time_to_nanos(t: time) -> int:
+ """Convert a datetime.time object to nanoseconds from midnight."""
+ # python datetime and time doesn't have nanoseconds support yet
+ # https://github.com/python/cpython/issues/59648
+ return ((((t.hour * 60 + t.minute) * 60) + t.second) * 1_000_000 + t.microsecond) * 1_000
+
+
+def datetime_to_nanos(dt: datetime) -> int:
+ """Convert a datetime to nanoseconds from 1970-01-01T00:00:00.000000000."""
+ # python datetime and time doesn't have nanoseconds support yet
+ # https://github.com/python/cpython/issues/59648
+ if dt.tzinfo:
+ delta = dt - EPOCH_TIMESTAMPTZ
+ else:
+ delta = dt - EPOCH_TIMESTAMP
+ return ((delta.days * 86400 + delta.seconds) * 1_000_000 + delta.microseconds) * 1_000
+
+
+def timestamp_to_nanos(timestamp_str: str) -> int:
+ """Convert an ISO-9601 formatted timestamp without zone to microseconds from 1970-01-01T00:00:00.000000.
+
+ Currently only microsecond precision timestamp_str is supported as python datetime does not have
+ nanoseconds support.
+ """
+ if ISO_TIMESTAMP.fullmatch(timestamp_str):
+ return datetime_to_nanos(datetime.fromisoformat(timestamp_str))
Review Comment:
Currently, the function will fail to parse timestamp string if it has nanosecond precision because it doesn't match the `ISO_TIMESTAMP` format. I was initially thinking of just separating that work out in a different PR, but I think it should be simple enough to address it here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2003186605
##########
pyiceberg/types.py:
##########
@@ -62,6 +63,12 @@
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+class TableVersion(IntEnum):
+ ONE = 1
+ TWO = 2
+ THREE = 3
Review Comment:
I don't see a ton of added value in addition to using a `TypeAlias` like we have in typedef:
https://github.com/apache/iceberg-python/blob/c06e32034fbd32d5aab9b2ef19ad2d253141479d/pyiceberg/typedef.py#L225
But I don't want to be that guy blocking this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r2002306704
##########
pyiceberg/types.py:
##########
Review Comment:
Thanks for chiming in @smaheshwar-pltr . Appreciate the review.
I read the last column that mentioned that promotion to timestamp and timestamp_N.A. is "not" allowed, but looking at the column for v3 spec, it seems to suggest otherwise. Hence I think this is worth clarifying with the community.
Referring to the Java code, I see that there is no valid promotion specified for `Date` type. Let me raise question in the dev list and clarify this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1977674291
##########
pyiceberg/transforms.py:
##########
@@ -323,6 +327,16 @@ def hash_func(v: Any) -> int:
return mmh3.hash(struct.pack("<q", v))
+ elif isinstance(source, (TimestampNanoType, TimestamptzNanoType)):
+
+ def hash_func(v: Any) -> int:
+ if isinstance(v, py_datetime.datetime):
+ v = datetime.datetime_to_micros(v)
Review Comment:
I'm wrong: https://github.com/apache/iceberg/blob/f14efce435980e0d46eadc7b8eb2f32ba22008d5/api/src/main/java/org/apache/iceberg/transforms/Bucket.java#L235-L247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1977669755
##########
pyiceberg/transforms.py:
##########
@@ -323,6 +327,16 @@ def hash_func(v: Any) -> int:
return mmh3.hash(struct.pack("<q", v))
+ elif isinstance(source, (TimestampNanoType, TimestamptzNanoType)):
+
+ def hash_func(v: Any) -> int:
+ if isinstance(v, py_datetime.datetime):
+ v = datetime.datetime_to_micros(v)
Review Comment:
I think this should be nano's:
```suggestion
if isinstance(v, py_datetime.datetime):
v = datetime.datetime_to_nanos(v)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] V3: Introduce `timestamp_ns` and `timestamptz_ns` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1632:
URL: https://github.com/apache/iceberg-python/pull/1632#discussion_r1977718277
##########
pyiceberg/utils/datetime.py:
##########
@@ -91,6 +91,57 @@ def timestamp_to_micros(timestamp_str: str) -> int:
raise ValueError(f"Invalid timestamp without zone: {timestamp_str} (must be ISO-8601)")
+def time_str_to_nanos(time_str: str) -> int:
+ """Convert an ISO-8601 formatted time to nanoseconds from midnight."""
+ return time_to_nanos(time.fromisoformat(time_str))
+
+
+def time_to_nanos(t: time) -> int:
+ """Convert a datetime.time object to nanoseconds from midnight."""
+ # python datetime and time doesn't have nanoseconds support yet
+ # https://github.com/python/cpython/issues/59648
+ return ((((t.hour * 60 + t.minute) * 60) + t.second) * 1_000_000 + t.microsecond) * 1_000
+
+
+def datetime_to_nanos(dt: datetime) -> int:
+ """Convert a datetime to nanoseconds from 1970-01-01T00:00:00.000000000."""
+ # python datetime and time doesn't have nanoseconds support yet
+ # https://github.com/python/cpython/issues/59648
+ if dt.tzinfo:
+ delta = dt - EPOCH_TIMESTAMPTZ
+ else:
+ delta = dt - EPOCH_TIMESTAMP
+ return ((delta.days * 86400 + delta.seconds) * 1_000_000 + delta.microseconds) * 1_000
+
+
+def timestamp_to_nanos(timestamp_str: str) -> int:
+ """Convert an ISO-9601 formatted timestamp without zone to microseconds from 1970-01-01T00:00:00.000000.
+
+ Currently only microsecond precision timestamp_str is supported as python datetime does not have
+ nanoseconds support.
+ """
+ if ISO_TIMESTAMP.fullmatch(timestamp_str):
+ return datetime_to_nanos(datetime.fromisoformat(timestamp_str))
Review Comment:
Here we truncate the nano's, which doesn't seem right. Maybe we need to do some post-processing to still add these to the `int`? WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org