SEPURI-SAI-KRISHNA commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4321209217
##########
superset/charts/client_processing.py:
##########
@@ -835,6 +840,19 @@ def union_currency_context(
}
+def format_time_column(df: pd.DataFrame, column: Any, d3_time_format: str) ->
None:
+ """Format a temporal column in place with an explicit d3 time format.
+
+ Only the directives shared between d3-time-format and ``strftime``
+ (``%Y``, ``%m``, ``%d``, ``%H``, ``%M``, ``%S`` and similar) are supported,
+ which covers the formats users configure in practice. Null values are left
+ as-is rather than rendered as the string ``"NaT"``.
+ """
+ df[column] = df[column].apply(
+ lambda value: value.strftime(d3_time_format) if pd.notna(value) else
value
Review Comment:
Agreed, and it is worse than a mismatch: `strftime` does not raise on a
directive it does not know, it copies it into the output.
```
%L '%L'
%q '%q'
%Q '%Q'
```
So the column is gated on an allowlist now, and anything outside it leaves
the column raw, the way the number path leaves a format it cannot handle:
```python
SHARED_TIME_DIRECTIVES = frozenset("aAbBdfGHIjmMpSuUVwWyY%")
def is_strftime_expressible(d3_time_format: str) -> bool:
"""Whether every directive in a d3 time format means the same to
strftime."""
directives = re.findall(r"%(.)", d3_time_format)
return bool(directives) and all(
directive in SHARED_TIME_DIRECTIVES for directive in directives
)
```
What I left out, with reasons, since the list is the whole argument:
`%c`, `%x`, `%X` are locale-composite. Python builds them from the server's
locale and d3 from the chart's, so `%x` gives `04/02/24` here and a different
order elsewhere. Those are a silent mismatch rather than a visible one, which
is worse.
`%L`, `%q`, `%Q` are d3-only, the ones above.
`%e` and `%s` work through the platform's C library rather than CPython, and
`%g` is not d3's two-digit ISO year. `%Z` and `%z` render empty for a naive
datetime here and offsets in the browser.
`%f`, `%G`, `%u`, `%V`, `%U`, `%W` and `%j` are in, having checked each
against d3-time-format's table rather than assuming: microseconds, ISO year,
ISO weekday, ISO week, the two week-of-year variants and day-of-year all agree.
`test_table_skips_a_time_format_strftime_cannot_express` covers `%Y Q%q`,
`%H:%M:%S.%L`, `%Q`, `%-d %b` and `%x`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
