sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4320330036
##########
superset/charts/client_processing.py:
##########
@@ -1237,6 +1255,27 @@ def label_of(name: str) -> str:
# left untouched, matching the browser.
for column in df.columns:
config = config_by_label.get(column) or {}
+
+ # Mirror the Table plugin's first formatter branch
+ # (``if (isTime || config.d3TimeFormat)`` in ``transformProps.ts``):
+ # an explicit per-column ``d3TimeFormat`` formats a temporal column and
+ # takes precedence over number formatting. Scoped to datetime columns
+ # with an explicit ``strftime``-style format; ``smart_date`` and any
+ # other alias is skipped so it is never rendered as literal text. The
+ # bare ``isTime`` fallback, ``tableTimestampFormat`` default,
+ # SMART_DATE granularity, and string temporal columns are left to a
+ # follow-up, since they need a full server-side d3-time-format port
+ # rather than ``strftime``.
+ time_format = config.get("d3TimeFormat")
+ if (
+ time_format
+ and time_format != SMART_DATE
+ and "%" in time_format
+ and pd.api.types.is_datetime64_any_dtype(df[column])
+ ):
+ format_time_column(df, column, time_format)
Review Comment:
The formatted strings don't survive to the report:
`extract_dataframe_dtypes` still tags this column `TEMPORAL` (it goes by the
dataset's `is_dttm`, not the new dtype), and `chart_data_to_dataframe` then
runs `astype("datetime64[ms]")` on it. With `%Y` the cells come back as
`2024-01-01`, and with `%d/%m/%Y` the string `02/01/2024` is re-parsed
month-first as Feb 1 instead of Jan 2. Should the formatted column be excluded
from that re-cast (or tagged non-temporal in `coltypes`) so the notification
actually shows the configured format?
##########
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:
The `%` check doesn't restrict the format to the directives shared with d3,
so d3-only ones are emitted literally: `%Y Q%q` renders as `2024 Q%q` and `%L`
stays `%L`, whereas the browser shows `2024 Q2` / milliseconds. Would it be
safer to leave the column raw when the format contains a directive `strftime`
can't express, like the number path does for formats it can't handle?
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2488,6 +2488,60 @@ def test_table_applies_currency_format() -> None:
assert formatted["amount"].tolist() == ["$ 1,234.50"]
+def test_table_applies_time_format_alongside_number_format() -> None:
Review Comment:
The PR description and `format_time_column` docstring promise that null
(`NaT`) is preserved, but both new tests use only valid timestamps, so removing
the `pd.notna` guard would pass them. Could you add a `table()` case with
`ds=[pd.Timestamp("2024-01-15"), pd.NaT]` and `d3TimeFormat="%Y"` asserting
`"2024"` and a null in the second cell?
--
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]
