You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "JE-Chen (via GitHub)" <gi...@apache.org> on 2024/09/25 13:41:55 UTC
[PR] PR #1169 [iceberg-python]
JE-Chen opened a new pull request, #1206:
URL: https://github.com/apache/iceberg-python/pull/1206
Did you mean to change it like this PR? @kevinjqliu
I think we need to edit the test as well.
Becasue test no longer raise ns TypeError.
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775623041
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
return TimeType(primitive)
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean return new TimeType using new primitive right?
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775666368
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
primitive = cast(pa.Time64Type, "us")
```
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775623041
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
return TimeType(primitive)
```
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1777324689
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
essentially check if the `unit` is `ns`. if it is, check `downcast-ns-timestamp-to-us-on-write`. if the option is set, we can downcast from `ns` to `us`.
If its not, raise an error
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775491517
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
return TimeType()
elif pa.types.is_timestamp(primitive):
primitive = cast(pa.TimestampType, primitive)
- if primitive.unit in ("s", "ms", "us"):
+ if primitive.unit in ("s", "ms", "us", "ns"):
# Supported types, will be upcast automatically to 'us'
pass
- elif primitive.unit == "ns":
- if self._downcast_ns_timestamp_to_us:
- logger.warning("Iceberg does not yet support 'ns' timestamp precision. Downcasting to 'us'.")
- else:
- raise TypeError(
- "Iceberg does not yet support 'ns' timestamp precision. Use 'downcast-ns-timestamp-to-us-on-write' configuration property to automatically downcast 'ns' to 'us' on write."
- )
Review Comment:
I will add back this code snippet.
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775666368
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
primitive = cast(pa.Time64Type, "us")
```
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775490550
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
> similar to below
Thank you for your response, but could you explain it more clearly?
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775734036
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean this?
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.Time64Type, primitive)
```
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1777321794
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
So this function is converting pyarrow types to Iceberg types.
In the example of timestamp type (`pa.types.is_timestamp(primitive)`), we check if the `unit` (or precision) is `ns`. If it is and the `downcast-ns-timestamp-to-us-on-write` is set, we want to downcast the `unit` from `ns` to `us`. If `downcast-ns-timestamp-to-us-on-write` is not set, raise an error because `ns` is not supported.
If its not, we don't do anything special and just return the Iceberg type.
We want to do the same thing with the `Time64Type` type.
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775500020
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
> dont need to check for precision here, since its us and ns are supported.
[Time64Type](https://arrow.apache.org/docs/python/generated/pyarrow.Time64Type.html) only supports `us` and `ns`, so the extra check for `primitive.unit in ["us", "ns"]` is redundant. Although, it'll be helpful if another precision was added in the future.
> Although we need to add code to downcast ns to us, similar to below
Because Iceberg doesn't support `ns`, it has to downcast `ns` to `us`, or errors.
This behavior is controlled by https://github.com/apache/iceberg-python/pull/1206/files#diff-8d5e63f2a87ead8cebe2fd8ac5dcf2198d229f01e16bb9e06e21f7277c328abdL1078-L1084
Which we also need to include 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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775623776
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean return new TimeType using new primitiveright?
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean return new TimeType using new primitive right?
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1776310431
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
I am confused.
Did you mean that I need a new class variable _downcast_ns_time64type_to_us and a new condition to check?
```
if self._downcast_ns_time64type_to_us:
# what should be done here?
```
Or should I add some code under the original condition:
```
if self._downcast_ns_timestamp_to_us:
# Add something 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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen closed pull request #1206: PR #1169
URL: https://github.com/apache/iceberg-python/pull/1206
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775623776
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean return new TimeType using new primitive.unit right?
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775623776
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
You mean return new Timetype using new primitive.unit right?
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775672026
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
cast(pa.Time64Type, "us")
```
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1777321794
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
So this function is converting pyarrow types to Iceberg types.
In the example of timestamp type (`pa.types.is_timestamp(primitive)`), we check if the `unit` (or precision) is `ns`. If it is and the `downcast-ns-timestamp-to-us-on-write` is set, we want to downcast the `unit` from `ns` to `us`.
If its not, we don't do anything special and just return the Iceberg type.
We want to do the same thing with the `Time64Type` type.
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1776008439
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
the `_downcast_ns_timestamp_to_us` part for sure. Im not sure if we need to do `cast` though
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1777551420
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
Please check the new commit. Thanks.
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775434612
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
return TimeType()
elif pa.types.is_timestamp(primitive):
primitive = cast(pa.TimestampType, primitive)
- if primitive.unit in ("s", "ms", "us"):
+ if primitive.unit in ("s", "ms", "us", "ns"):
# Supported types, will be upcast automatically to 'us'
pass
- elif primitive.unit == "ns":
- if self._downcast_ns_timestamp_to_us:
- logger.warning("Iceberg does not yet support 'ns' timestamp precision. Downcasting to 'us'.")
- else:
- raise TypeError(
- "Iceberg does not yet support 'ns' timestamp precision. Use 'downcast-ns-timestamp-to-us-on-write' configuration property to automatically downcast 'ns' to 'us' on write."
- )
Review Comment:
this is still necessary because iceberg doesnt support `ns`. this workaround help us downcast a column with `ns` to `us`
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
dont need to check for precision here, since its `us` and `ns` are supported.
Although we need to add code to downcast `ns` to `us`, similar to below
--
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] PR #1169 [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775613883
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
yep, do the same for `Time64Type`
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775672026
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
```
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
cast(pa.Time64Type, "us")
```
--
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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1776310431
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
Thank you for your response, But I am confused.
Did you mean that I need a new class variable _downcast_ns_time64type_to_us and a new condition to check?
```
if self._downcast_ns_time64type_to_us:
# what should be done here?
```
Or should I add some code under the original condition:
```
if self._downcast_ns_timestamp_to_us:
# Add something 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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1776310431
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
I am confused.
Did you mean that I need a new class variable _downcast_ns_time64type_to_us and a new condition to check?
```
if self._downcast_ns_time64type_to_us:
# what should be done here?
```
Or should I add some action under the original condition:
```
if self._downcast_ns_timestamp_to_us:
# Add something 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] PR #1169 [iceberg-python]
Posted by "JE-Chen (via GitHub)" <gi...@apache.org>.
JE-Chen commented on code in PR #1206:
URL: https://github.com/apache/iceberg-python/pull/1206#discussion_r1775612249
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1068,20 +1068,13 @@ def primitive(self, primitive: pa.DataType) -> PrimitiveType:
return StringType()
elif pa.types.is_date32(primitive):
return DateType()
- elif isinstance(primitive, pa.Time64Type) and primitive.unit == "us":
+ elif isinstance(primitive, pa.Time64Type) and primitive.unit in ["us", "ns"]:
Review Comment:
Like this?
```
elif pa.types.is_timestamp(primitive):
primitive = cast(pa.TimestampType, primitive)
if primitive.unit in ("s", "ms", "us"):
# Supported types, will be upcast automatically to 'us'
pass
elif primitive.unit == "ns":
if self._downcast_ns_timestamp_to_us:
primitive = cast(pa.TimestampType, "us")
else:
raise TypeError(
"Iceberg does not yet support 'ns' timestamp precision. Use 'downcast-ns-timestamp-to-us-on-write' configuration property to automatically downcast 'ns' to 'us' on write."
)
```
--
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