Core: Deprecate 'downcast-ns-timestamp-to-us-on-write' config key - #3596
Core: Deprecate 'downcast-ns-timestamp-to-us-on-write' config key#3596qzyu999 wants to merge 3 commits into
Conversation
59ade22 to
c887117
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Hi @sungwy, PTAL when available |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
I think this is fine. I've got one thing around the deprecation message, but this looks fine otherwise. Thanks for doing this!
| StagedTable, | ||
| Table, | ||
| TableProperties, | ||
| _get_downcast_ns_timestamp_to_us, |
There was a problem hiding this comment.
Can we make this a public method?
I personally don't like importing private methods.
|
|
||
| legacy = config.get_bool(DOWNCAST_NS_TIMESTAMP_TO_US_ON_WRITE) | ||
| if legacy is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
Please use our built-in deprecation message instead. It helps release managers know when to remove things:
From docs:
from pyiceberg.utils.deprecated import deprecation_message
deprecation_message(
deprecated_in="0.1.0",
removed_in="0.2.0",
help_message="The old_property is deprecated. Please use the something_else property instead.",
)
|
Hi @rambleraptor, thanks for the review, all the changes have been made. |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
One quick nit on the versions. Thanks again!
|
|
||
| ALWAYS_TRUE = AlwaysTrue() | ||
| DOWNCAST_NS_TIMESTAMP_TO_US = "downcast-ns-timestamp-to-us" | ||
| # Deprecated: use DOWNCAST_NS_TIMESTAMP_TO_US. The old key said "on-write" but the |
There was a problem hiding this comment.
We don't need this full comment. Just Deprecated: use DOWNCAST_NS_TIMESTAMP_TO_US
| if legacy is not None: | ||
| deprecation_message( | ||
| deprecated_in="0.9.0", | ||
| removed_in="0.10.0", |
There was a problem hiding this comment.
This isn't the right version. We should deprecate in 0.12.0 (assuming this goes in this release) and remove in 0.13.0
There was a problem hiding this comment.
Thanks for confirming, I should've double-checked previously, made the changes.
…of 'downcast-ns-timestamp-to-us' The config key 'downcast-ns-timestamp-to-us-on-write' says 'on-write' but is also used in the read path (ArrowScan). Rename the canonical key to 'downcast-ns-timestamp-to-us' which accurately describes the behavior regardless of direction. The old key still works but emits a DeprecationWarning. The new key takes precedence when both are set.
735d408 to
c541c98
Compare
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Looks great. Especially love the docs. Thanks for doing this!
2 participants
Which issue does this PR close?
Minor config naming issue raised in #3594 (review)
Rationale for this change
The config key
downcast-ns-timestamp-to-us-on-write(env:PYICEBERG_DOWNCAST_NS_TIMESTAMP_TO_US_ON_WRITE) says 'on-write' but is also used in the read path (ArrowScan) to downcast nanosecond timestamps to microseconds. The name is misleading.What changes are included in this PR?
downcast-ns-timestamp-to-us(env:PYICEBERG_DOWNCAST_NS_TIMESTAMP_TO_US)_get_downcast_ns_timestamp_to_us()helper that checks the new key first, falls back to the old key with aDeprecationWarning\Are there any user-facing changes?
downcast-ns-timestamp-to-us/PYICEBERG_DOWNCAST_NS_TIMESTAMP_TO_US\DeprecationWarning\