SEPURI-SAI-KRISHNA commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4326580767
##########
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:
You are right, and the mutation you describe is worse than passing: it is
silent. Returning `STRING` for every temporal column instead of only a
text-valued one passed all 126 tests on the previous commit.
```
unformatted datetime column
as written coltypes = ['TEMPORAL', 'STRING']
STRING for every temporal coltypes = ['STRING', 'STRING']
formatted datetime column
both coltypes = ['STRING', 'STRING']
```
Added both cases you asked for.
`test_report_payload_keeps_an_unformatted_temporal_column_temporal` runs a
payload with no `d3TimeFormat` through `apply_client_processing` and asserts
the
column stays `TEMPORAL` and its values stay `Timestamp`s. That is the test
the
mutation now fails.
`test_report_payload_types_a_csv_read_temporal_column_as_text` is the other
half, and checking it turned up something I had not registered: `coltypes` is
built for every result format, while `apply_number_format` is only true for
JSON. So the CSV branch never formats anything, its temporal column arrives
as
text straight from `read_csv`, and the override fires on a column no
post-processor touched:
```
CSV payload "ds,cnt\n2024-01-02,1\n2024-01-15,2\n"
coltypes = ['STRING', 'NUMERIC'] and the CSV text is handed back
untouched
```
Which is the behavior I want, since the type should describe the values
rather
than record whether this module wrote them, but it was worth pinning
precisely
because the obvious "fix" if someone saw that in isolation would be to key
the
override on whether a format was applied. The test says so in as many words.
Between the two, each direction is now caught: dropping the override fails
the
formatted case and the CSV case, and widening it to every temporal column
fails
the unformatted one.
One thing I checked while in there and left alone: on the XLSX branch the
override is inert, because `_read_excel_for_client_processing` takes the
first
column as the index, so that payload reports `['NUMERIC']` with
`colnames: ['cnt']` both with and without this change. Pre-existing and
unrelated, just noting it so it does not look like a gap this introduces.
--
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]
