Fix Polars temporal column stats JSON serialization for tracker - #1675
Fix Polars temporal column stats JSON serialization for tracker#1675dgvj-work wants to merge 2 commits into
Conversation
a65d947 to
670e8f4
Compare
Datetime tracking for apache#1127 already avoids std(); extend the same datetime stats path so Time and Duration values serialize cleanly, and add a Datetime regression test matching the reported failure.
670e8f4 to
66cb544
Compare
jernejfrank
left a comment
jernejfrank
left a comment
There was a problem hiding this comment.
Hey, just a small thing since not everything renders on UI
| def _temporal_to_jsonable(value: object) -> object: | ||
| # datetime.datetime is a subclass of datetime.date. Time/Duration also arrive via | ||
| # selectors.temporal() and must be JSON-safe for the UI (same base_data_type="datetime"). | ||
| if isinstance(value, (datetime.date, datetime.time)): | ||
| return value.isoformat() | ||
| if isinstance(value, datetime.timedelta): | ||
| return str(value) | ||
| return value | ||
|
|
||
|
|
There was a problem hiding this comment.
This breaks a couple of daterendering on the UI:
"2021-01-01T00:00:00" → valid Date
"01:00:00" → Invalid Date / NaN
"1 day, 0:00:00" → Invalid Date / NaN
"2 days, 0:00:00" → Invalid Date / NaN
Can we route Time/Duration to unhandled stats for now?
There was a problem hiding this comment.
@jernejfrank Good catch, those Time/Duration strings are not valid JS Dates, so they were breaking the datetime range renderer.
I routed Time and Duration to unhandled stats instead of datetime stats. Date/Datetime still go through the datetime path with ISO timestamps (2021-01-01T00:00:00), which new Date(...) can parse.
The #1127 Datetime regression coverage is still there, plus a test that Time/Duration stay on the unhandled path (no min/max date strings).
4fe0f3f to
e423a0d
Compare
Time and Duration values are not valid JS Dates, so sending them through datetime stats breaks the UI range renderer. Keep Date/Datetime on the ISO datetime path and leave Time/Duration for unhandled stats.
e423a0d to
463ffc6
Compare
|
@jernejfrank can you please review the changes. |
2 participants
Make Polars Date/Datetime column stats JSON-safe for the tracker, and keep Time/Duration off the datetime UI path.
Changes
datetime_column_statswith ISO timestamps (base_data_type="datetime")"01:00:00"/"1 day, 0:00:00"as DatesHow I tested this
pytest ui/sdk/tests/tracking/test_polars_col_stats.py ui/sdk/tests/tracking/test_polars_stats.py(22 passed)ruff formaton the touched files (passed)Notes
std()crash by routing throughdatetime_column_statswithstd=0.0, but Hamilton tracker gives an error with datetime columns in polars dataframes #1127 was still open: there was no regression test pinning that Datetime path. This PR closes that loop without sending Time/Duration through the datetime renderer.Closes #1127
Checklist