sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4326159037


##########
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

Review Comment:
   This STRING override is the only new branch in the payload types, but 
nothing pins its other half. The only assertion on `coltypes` is the formatted 
case, so if every temporal column were reported as `STRING`, an untouched 
`is_dttm` datetime column would lose its `TEMPORAL` type, and the XLSX path 
would stop writing date cells for it, and every test would still pass. Could 
you add a case through `apply_client_processing` with an unformatted datetime 
column asserting it stays `TEMPORAL`, and one with a CSV-read (string) temporal 
column to show what it reports?



-- 
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]

Reply via email to