feat: New zoned timestamp feature type (#6536)#6537
Conversation
Signed-off-by: Nick Quinn <nicholas_quinn@apple.com>
franciscojavierarceo
left a comment
There was a problem hiding this comment.
can you update the PR description?
Sure! @franciscojavierarceo , I noticed that devin is no longer intregrated into PRs. |
|
@codex review |
|
yeah i need to tune it because we burned through our quota too quickly 😓 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7243418383
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| StringList decimal_list_val = 45; | ||
| StringSet decimal_set_val = 46; | ||
| ScalarMap scalar_map_val = 47; | ||
| ZonedTimestamp zoned_timestamp_val = 48; |
There was a problem hiding this comment.
Add JSON serialization for zoned timestamp values
When a ZONED_TIMESTAMP feature is returned by the feature server, this new oneof makes Value.WhichOneof("val") return zoned_timestamp_val, but feature_server_utils._value_to_native() and the proto_json patch only special-case lists/maps/scalars and otherwise return the raw protobuf message. feature_server.py then passes that dict to JSONResponse, which cannot serialize a ZonedTimestamp message, so any online response containing this new type fails instead of returning a JSON value.
Useful? React with 👍 / 👎.
| Uuid, | ||
| TimeUuid, | ||
| Decimal, | ||
| ZonedTimestamp, |
There was a problem hiding this comment.
Do not enable ZonedTimestamp collections without enum support
Adding ZonedTimestamp to SUPPORTED_BASE_TYPES makes Array(ZonedTimestamp) and Set(ZonedTimestamp) pass construction, but Array.to_value_type() and Set.to_value_type() derive ValueType["ZONED_TIMESTAMP_LIST"] / ValueType["ZONED_TIMESTAMP_SET"], which are not defined in value_type.py or Value.proto. Any schema using this new primitive in an array/set will raise a KeyError during apply/serialization; either add the list/set value types and converters or exclude it from supported collection base types.
Useful? React with 👍 / 👎.
| except Exception: | ||
| # Not an IANA name (e.g. a fixed-offset string we couldn't resolve); | ||
| # fall back to UTC rather than failing the whole read. | ||
| logger.warning("Could not resolve zone %r; decoding as UTC", zone) | ||
| return timezone.utc |
There was a problem hiding this comment.
Preserve fixed-offset zones when decoding
For datetimes whose tzinfo is a fixed offset, _zone_name() stores strings like UTC-07:00 (and the proto comment also advertises fixed-offset strings such as -07:00), but _zone_from_name() only tries ZoneInfo(zone) and then falls back to UTC. In that scenario a round trip changes the wall-clock time and loses the original offset, which breaks the advertised zone preservation for fixed-offset datetimes.
Useful? React with 👍 / 👎.
| DECIMAL_LIST = 45 | ||
| DECIMAL_SET = 46 | ||
| SCALAR_MAP = 47 | ||
| ZONED_TIMESTAMP = 48 |
There was a problem hiding this comment.
Add ZonedTimestamp to dtype string conversions
After adding this enum, _convert_value_type_str_to_value_type() still does not recognize "ZONED_TIMESTAMP" and defaults unknown strings to ValueType.STRING. Registry REST helpers such as feature_views.extract_feast_types_from_fields() and data-source code generation call that converter on valueType, so a registry field with this new type is rendered/handled as String instead of ZonedTimestamp.
Useful? React with 👍 / 👎.
Adding new zoned timestamp feature type that stores both timestamp (int64) and zone info (str). If zone is UTC, it only stores timestamp. This is similar to ZonedDateTime in java. This proto type will map to
datetimewith zoneinfo to load for configured zone. For example,