You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kevinzwang (via GitHub)" <gi...@apache.org> on 2024/09/25 21:33:38 UTC
[PR] fix: DayTransform result type override and docs [iceberg-python]
kevinzwang opened a new pull request, #1208:
URL: https://github.com/apache/iceberg-python/pull/1208
`DayTransform.result_type()` incorrectly returns `DateType()`. This just removes that so it has the same behavior as other time transforms, returning an `IntegerType()`. The actual return values of `DayTransform.transform` and `DayTransform.pyarrow_transform` have been correct
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2378238683
devlist https://lists.apache.org/thread/2gq7b54nvc9q6f1j08l9lnzgm5onkmx5
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2377581839
It's more readable sure, but just does not conform to the spec. I'm not entirely sure what you mean by your question @kevinjqliu, I believe partition values are only stored in metadata.
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375566385
Im not 100% sure, perhaps the metadata table does the transformation.
https://iceberg.apache.org/docs/latest/spark-queries/#partitions
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2377716666
The partition value is stored in the metadata as int, but the "partition metadata table" (https://iceberg.apache.org/docs/latest/spark-queries/#partitions) shows the partition data as a "timestamp" instead.
When querying the "partition metadata table", the int partition value is transformed to its "timestamp" representation.
This is the difference between spark and pyiceberg, as seen in the failed test, `test_inspect_partitions_partitioned`.
So perhaps the "partition metadata table" is implemented wrong.
To verify, we need to find where the transformation, from int to timestamp, happens on the spark side
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375801600
> Im not 100% sure, perhaps the metadata table does the transformation.
>
> https://iceberg.apache.org/docs/latest/spark-queries/#partitions
I think you are correct.
Physically, the integer value stored is days since epoch. But Spark encodes the type of the day transformed partition field in the metadata file as date type. On the other hand, for other transforms Spark conforms to the Iceberg spec. Strange.
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on code in PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#discussion_r1776140899
##########
pyiceberg/transforms.py:
##########
@@ -517,9 +517,6 @@ def day_func(v: Any) -> int:
def can_transform(self, source: IcebergType) -> bool:
return isinstance(source, (DateType, TimestampType, TimestamptzType))
- def result_type(self, source: IcebergType) -> IcebergType:
- return DateType()
-
Review Comment:
I'm matching the behavior of the other transforms that extend `TimeTransform`, which all just implicitly use `TimeTransform.result_type` instead of overriding it. Should we change this for all of them?
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2378222143
> Basically, Spark stores day transformed partition values incorrectly in the metadata
Thats an interesting find... The core Iceberg library is using `DateType` as the Result Type for DayTransform
https://github.com/apache/iceberg/blame/main/api/src/main/java/org/apache/iceberg/transforms/Days.java#L47
In fact, its the only "time-based" transforms which uses this
https://grep.app/search?q=getResultType&filter[repo][0]=apache/iceberg&filter[path][0]=api/src/main/java/org/apache/iceberg/
Let me bring this up in the devlist. In the meantime, I think we can either
1. Put back `DateType` as the result type for Day Transform to match the behavior
2. Fix it in the Partitions metadata table as a workaround and link an issue to fix forward
I don't want to a workaround in the tests as it will become difficult to maintain. WYDT?
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#discussion_r1776147728
##########
pyiceberg/transforms.py:
##########
@@ -517,9 +517,6 @@ def day_func(v: Any) -> int:
def can_transform(self, source: IcebergType) -> bool:
return isinstance(source, (DateType, TimestampType, TimestamptzType))
- def result_type(self, source: IcebergType) -> IcebergType:
- return DateType()
-
Review Comment:
gotcha, thanks for the context. This is fine!
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375442625
> is this the source of truth? https://iceberg.apache.org/spec/#partition-transforms
Yup, precisely
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2377884626
I played around with Spark and inspected the generated metadata. I believe the partition value is actually stored in the metadata as date type for day transformed partitions.
The reason why we are getting the right value but just as an integer in the test is that the physical type of date type is integer, and pyiceberg coerces the metadata table to the types from `Transform.result_type` upon reading them.
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375810823
I like that its converted, its more readable! Do you know where the transform happens? Is it only for the metadata table?
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#discussion_r1776111931
##########
pyiceberg/transforms.py:
##########
@@ -517,9 +517,6 @@ def day_func(v: Any) -> int:
def can_transform(self, source: IcebergType) -> bool:
return isinstance(source, (DateType, TimestampType, TimestamptzType))
- def result_type(self, source: IcebergType) -> IcebergType:
- return DateType()
-
Review Comment:
nit: can we be explicit here and do
```
def result_type(self, source: IcebergType) -> IntegerType:
return IntegerType()
```
##########
pyiceberg/transforms.py:
##########
@@ -491,7 +491,7 @@ class DayTransform(TimeTransform[S]):
"""Transforms a datetime value into a day value.
Example:
- >>> transform = MonthTransform()
+ >>> transform = DayTransform()
Review Comment:
ty!
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375530341
Ok so interesting... Spark actually does store day transforms as date type in the metadata, which is why the integration test is failing. This is probably why this library had that behavior before. So Spark itself does not follow the Iceberg spec. Thoughts on deviating from Spark behavior?
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2375337828
is this the source of truth?
https://iceberg.apache.org/spec/#partition-transforms
--
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] fix: DayTransform result type override and docs [iceberg-python]
Posted by "kevinzwang (via GitHub)" <gi...@apache.org>.
kevinzwang commented on PR #1208:
URL: https://github.com/apache/iceberg-python/pull/1208#issuecomment-2379885390
Perhaps let's just wait on the response on the devlist first.
--
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