SEPURI-SAI-KRISHNA commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4321181830
##########
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:
You are right, and this was the finding that mattered most. The formatted
strings did not reach the recipient at all. I measured it before fixing
anything:
```
dtype of ds column : object
coltypes : ['TEMPORAL', 'NUMERIC']
notification table ->
ds cnt
0 2024-02-01 1
1 2024-01-15 2
```
That is `%d/%m/%Y` over `02/01/2024` and `15/01/2024`. The first is
re-parsed month-first and the second day-first, because `15` cannot be a month,
so a single column comes out with its dates inconsistent rather than uniformly
shifted. `%Y` collapses to `2024-01-01` as you said.
The type now follows the values:
```python
coltypes = extract_dataframe_dtypes(df, datasource)
return [
GenericDataType.STRING
if coltype == GenericDataType.TEMPORAL
and pd.api.types.infer_dtype(df.iloc[:, index], skipna=True) ==
"string"
else coltype
for index, coltype in enumerate(coltypes)
]
```
Two things I checked before settling on that shape rather than changing the
cast in `chart_data_to_dataframe`:
`coltypes` is not inert for every format. The XLSX branch passes it to
`excel.apply_column_types` to get real date cells, so a blanket "a text column
is not temporal" rule would have turned XLSX dates into strings. Testing the
values rather than the format keeps that path untouched: the frame it hands
over still holds datetimes, read back by `read_excel`, so nothing is
downgraded. A plain temporal column on the JSON path is also untouched, since
`DataFrame.from_dict` over the in-process rows gives it a `datetime64` dtype.
The cast itself has to stay. It exists because the payload's temporal values
are not datetimes by the time the notification table reads them, which is still
true for every column this PR does not format.
`test_table_report_time_format_survives_the_notification_table` now runs the
whole path, from `apply_client_processing` through `chart_data_to_dataframe`,
and asserts the cells are still `02/01/2024` and `15/01/2024` at the end. On
the parent commit it fails at `coltypes[0] == STRING` with `TEMPORAL`.
--
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]
