sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4333433561
##########
superset/charts/client_processing.py:
##########
@@ -1237,6 +1302,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 is_strftime_expressible(time_format)
+ and pd.api.types.is_datetime64_any_dtype(df[column])
Review Comment:
The description says a non-datetime column carrying `d3TimeFormat` is left
untouched, but every new `%Y` test passes datetime input, so deleting this
`is_datetime64_any_dtype` guard would keep them all green while a string or
numeric column with that key would reach `.strftime` and raise `AttributeError`
in the report. Could you add a `table()` case with a string (or integer) column
configured with `d3TimeFormat="%Y"` asserting it comes back unchanged?
##########
superset/charts/client_processing.py:
##########
@@ -1304,6 +1390,34 @@ def _read_excel_for_client_processing(
return df.set_index(first_column)
+def payload_coltypes(
+ df: pd.DataFrame,
+ datasource: Optional[Union["BaseDatasource", "Query"]],
+) -> list[GenericDataType]:
+ """The payload's column types, reporting a text-rendered column as text.
+
+ ``extract_dataframe_dtypes`` lets the dataset's ``is_dttm`` flag decide the
+ type, so a temporal column a post-processor has rendered as text is still
+ reported as ``TEMPORAL``. ``chart_data_to_dataframe`` casts every
+ ``TEMPORAL`` column back to ``datetime64[ms]`` to build the notification
+ table, which would re-parse those strings and lose the configured format,
+ and would read ``02/01/2024`` month-first while reading ``15/01/2024``
+ day-first.
+
+ Only a column whose values really are strings is reported differently. The
+ XLSX path hands these types to ``apply_column_types`` to get date cells out
+ of a frame whose dates are still dates, and that frame is untouched here.
+ """
+ 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"
Review Comment:
A formatted temporal column that also contains a null never gets the
`STRING` type: `infer_dtype(["02/01/2024", pd.NaT], skipna=True)` returns
`"mixed"` (pandas 2.3.3), not `"string"`, because `format_time_column` leaves
the `NaT` in place. The column stays `TEMPORAL`, and `chart_data_to_dataframe`
then casts it back to `datetime64[ms]`, reading `02/01/2024` month-first as
2024-02-01 (I ran that cast). So with `%d/%m/%Y` on `[2024-01-02, null]` the
report shows a different date, and any null in the column defeats the fix
described above. Could the check look only at the non-null values, or could the
formatted columns be tracked explicitly, with a test that has a `NaT` alongside
the string?
--
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]
