Add clarifying docs to transform result types #1211
Conversation
pyiceberg/transforms.py
Outdated
| def result_type(self, source: IcebergType) -> IcebergType: ... | ||
| def result_type(self, source: IcebergType) -> IcebergType: | ||
| """ | ||
| Return the type of the result of the transform. |
There was a problem hiding this comment.
| Return the type of the result of the transform. | |
| Returns the `IcebergType` produced by this transform given a source type. |
|
CI is failing due to dead loom link, removing here #1213 |
pyiceberg/transforms.py
Outdated
| """ | ||
| Return the type of the result of the transform. | ||
|
|
||
| This type does not need to conform to the Iceberg spec, as long as it is converted to the correct type in the stored metadata. |
There was a problem hiding this comment.
reading this as someone less familiar with the discussions in the other MR, this doesn't tell me much. Would it be good to link that MR here so future folks have the context of the discussion? Otherwise, might be good to go into more detail here.
There was a problem hiding this comment.
+1, I agree it's not capturing the whole context, let me see if I can come up with a suggestion.
There was a problem hiding this comment.
@kevinjqliu just following up on this. Do you have an alternative in mind? Otherwise, I will link the relevant PR here.
pyiceberg/transforms.py
Outdated
| """ | ||
| Return the type of the result of the transform. | ||
|
|
||
| This type does not need to conform to the Iceberg spec, as long as it is converted to the correct type in the stored metadata. |
There was a problem hiding this comment.
| This type does not need to conform to the Iceberg spec, as long as it is converted to the correct type in the stored metadata. | |
| This method defines both the physical and display representation of the partition field. | |
| The physical representation must conform to the Iceberg spec. The display representation | |
| can deviate from the spec, such as by transforming the value into a more human-readable format. |
There was a problem hiding this comment.
@kevinzwang @corleyma wdyt? does this capture the context?
There was a problem hiding this comment.
if this is good, maybe also update the DayTranform result_type function
something like,
The physical representation conforms to the Iceberg spec as DateType is internally
converted to int. The DateType returned here provides a more human-readable way to
display the partition field.
There was a problem hiding this comment.
This makes sense to me!
|
@kevinjqliu @corleyma updated the doc strings! Let me know if there's any other changes I should make |
|
CI is blocked on #1213, i'll work to resolve it |
|
#1213 is merged, so the CI issue should be fixed |
|
@kevinjqliu oh I merged instead of rebase. Either way, I think it makes sense to squash and merge this PR into main anyway |
|
Also I don't think I have perms to merge myself so feel free to push the button whenever |
|
Merged! Thanks @kevinzwang for the contribution! |
* add clarifying docs to transform result types * add more context to docs * re-add fixed first line * fix lint
* add clarifying docs to transform result types * add more context to docs * re-add fixed first line * fix lint
Follow-up from #1208, documents the conclusions from the discussion.