sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4402941108
##########
superset/charts/client_processing.py:
##########
@@ -1568,6 +1676,44 @@ 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.
+
+ Nulls are dropped before the values are read. ``format_time_column`` leaves
+ them in place, and ``infer_dtype`` counts a ``NaT`` as a value of its own
+ type rather than one ``skipna`` drops, so it calls a formatted column
+ holding one ``"mixed"``. A column of nothing but nulls reads as neither
+ text nor mixed and keeps whatever type the dataset gives it.
+
+ The index is positional because ``extract_dataframe_dtypes`` walks the
+ frame the same way, so the two line up even where a label repeats.
+ """
+ coltypes = extract_dataframe_dtypes(df, datasource)
+ return [
+ GenericDataType.STRING
+ if coltype == GenericDataType.TEMPORAL
+ and pd.api.types.infer_dtype(df.iloc[:, index].dropna(), skipna=False)
Review Comment:
The docstring says a column of nothing but nulls keeps the type the dataset
gives it, but no test pins that. Changing `== "string"` to `in ("string",
"empty")` leaves every current test green, because the null test mixes a string
in with the `NaT`. An all-`None` object column flagged `is_dttm` would then be
typed `STRING` instead of `TEMPORAL`. Could a direct `payload_coltypes` case
with only null values assert `[GenericDataType.TEMPORAL]`?
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2875,6 +2877,551 @@ def test_table_applies_currency_format() -> None:
assert formatted["amount"].tolist() == ["$ 1,234.50"]
+def test_table_applies_time_format_alongside_number_format() -> None:
+ """
+ Table reports honor an explicit ``d3TimeFormat`` on a temporal column,
+ mirroring the Table plugin's first formatter branch, while still applying
+ number formats on the same call. Without the time branch the timestamp was
+ left raw even though the browser formats it.
+ """
+ df = pd.DataFrame.from_dict(
+ {
+ "ds": {
+ 0: pd.Timestamp("2024-01-15 13:45:00"),
+ 1: pd.Timestamp("2024-06-30 08:00:00"),
+ },
+ "SUM(num)": {0: 1234567.0, 1: 42.0},
+ }
+ )
+ form_data = {
+ "viz_type": "table",
+ "metrics": ["SUM(num)"],
+ "column_config": {
+ "ds": {"d3TimeFormat": "%Y"},
+ "SUM(num)": {"d3NumberFormat": ",.2f"},
+ },
+ }
+ formatted = table(df, form_data)
+ assert formatted["ds"].tolist() == ["2024", "2024"]
+ assert formatted["SUM(num)"].tolist() == ["1,234,567.00", "42.00"]
+
+
+def test_table_leaves_a_null_time_format_alone() -> None:
+ """
+ A ``d3TimeFormat`` of JSON ``null`` leaves the column alone.
+
+ ``column_config`` is free-form JSON out of the saved chart params, so the
+ key can be present and null. ``config.get("d3TimeFormat", "").strip()``
+ would raise ``AttributeError: 'NoneType' object has no attribute 'strip'``
+ and fail the whole report, and every other table test here passes either
+ way because none of them puts a null in the key.
+ """
+ df = pd.DataFrame.from_dict(
+ {
+ "ds": {0: pd.Timestamp("2024-04-02 13:45:00")},
+ "SUM(num)": {0: 1234.5678},
+ }
+ )
+ form_data = {
+ "viz_type": "table",
+ "metrics": ["SUM(num)"],
+ "column_config": {
+ "ds": {"d3TimeFormat": None},
+ "SUM(num)": {"d3NumberFormat": ",.1f"},
+ },
+ }
+ formatted = table(df, form_data)
+ assert formatted["ds"].tolist() == [pd.Timestamp("2024-04-02 13:45:00")]
+ assert formatted["SUM(num)"].tolist() == ["1,234.6"]
+
+
[email protected](
+ ("time_format", "expected"),
+ [
+ ("%d", "01"),
+ ("%G", "2020"),
+ ("%H", "13"),
+ ("%I", "01"),
+ ("%j", "001"),
+ ("%m", "01"),
+ ("%M", "05"),
+ ("%S", "09"),
+ ("%u", "5"),
+ ("%U", "00"),
+ ("%V", "53"),
+ ("%w", "5"),
+ ("%W", "00"),
+ ("%y", "21"),
+ ("%Y", "2021"),
+ ("%%", "%"),
+ ("%G-W%V-%u", "2020-W53-5"),
+ ],
+)
+def test_table_time_format_matches_d3_for_each_shared_directive(
+ time_format: str, expected: str
+) -> None:
+ """
+ Every directive in ``SHARED_TIME_DIRECTIVES`` is held to d3's own output.
+
+ The expected strings are what ``utcFormat`` from ``d3-time-format`` returns
+ for this timestamp, so taking a directive out of the allowlist fails the
+ case that names it rather than passing unnoticed. 1 January 2021 is a
+ Friday in ISO week 53 of 2020, which is what separates ``%G`` from ``%Y``
+ and ``%V`` from ``%U`` and ``%W``; 13:05:09 separates ``%I`` from ``%H``.
+ """
+ df = pd.DataFrame.from_dict({"ds": {0: pd.Timestamp("2021-01-01
13:05:09")}})
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": time_format}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist() == [expected]
+
+
[email protected](
+ "time_format",
+ [
+ "%Y%",
+ "%d/%m/%Y%",
+ "%%%",
+ "%Y%%%",
+ ],
+)
+def test_table_skips_a_time_format_with_an_unconsumed_percent(
+ time_format: str,
+) -> None:
+ """
+ A ``%`` that no directive consumes is dropped by d3 and copied by
+ ``strftime``.
+
+ ``%Y%`` is ``2021`` in the chart and would be ``2021%`` in the report, and
+ ``%%%`` is ``%`` against ``%%``. Every directive in these is shared, so the
+ allowlist alone accepts them and the column has to be left to the browser
+ on the stray ``%`` instead.
+ """
+ df = pd.DataFrame.from_dict({"ds": {0: pd.Timestamp("2021-01-01
13:05:09")}})
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": time_format}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist() == [pd.Timestamp("2021-01-01 13:05:09")]
+
+
+def test_table_leaves_adaptive_time_format_alone() -> None:
+ """
+ ``smart_date`` ("Adaptive formatting") is an alias the browser resolves to
a
+ formatter, not a format string. It carries no ``%`` directive, so the rule
+ that a format has to have one is what keeps it away from ``strftime``,
+ which would hand the alias back unchanged and print it in every cell.
+ """
+ df = pd.DataFrame.from_dict(
+ {
+ "ds": {
+ 0: pd.Timestamp("2024-01-15 13:45:00"),
+ 1: pd.Timestamp("2024-06-30 08:00:00"),
+ },
+ }
+ )
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": "smart_date"}},
+ }
+ formatted = table(df, form_data)
+ assert formatted["ds"].tolist() == [
+ pd.Timestamp("2024-01-15 13:45:00"),
+ pd.Timestamp("2024-06-30 08:00:00"),
+ ]
+
+
[email protected](
+ "time_format",
+ [
+ "yyyy-mm-dd",
+ "Q1",
+ "Date",
+ "100%",
+ ],
+)
+def test_table_skips_a_time_format_with_no_directive(time_format: str) -> None:
+ """
+ A format carrying no ``%`` directive is not a format string.
+
+ ``strftime`` has nothing to substitute and returns the string it was given,
+ so accepting one of these would put ``yyyy-mm-dd`` in every cell of the
+ column instead of a date. This is the rule that also rejects the adaptive
+ alias.
+ """
+ df = pd.DataFrame.from_dict({"ds": {0: pd.Timestamp("2024-04-02
13:45:00")}})
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": time_format}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist() == [pd.Timestamp("2024-04-02 13:45:00")]
+
+
[email protected](
+ ("values", "expected"),
+ [
+ pytest.param(
+ {0: "2024-01-15", 1: "2024-06-30"},
+ ["2024-01-15", "2024-06-30"],
+ id="string column",
+ ),
+ pytest.param({0: 2024, 1: 2025}, [2024, 2025], id="integer column"),
+ ],
+)
+def test_table_skips_a_time_format_on_a_non_datetime_column(
+ values: dict[int, Any], expected: list[Any]
+) -> None:
+ """
+ ``d3TimeFormat`` on a column that is not datetime leaves the column alone.
+
+ A string or a number has no ``strftime`` to call, so formatting one would
+ raise ``AttributeError`` in the report worker. The browser reaches these
+ through a d3-time-format port, which parses the value first, and that is
+ what the follow-up needs rather than something ``strftime`` can stand in
+ for.
+ """
+ df = pd.DataFrame.from_dict({"ds": values})
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": "%Y"}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist() == expected
+
+
+def test_table_time_format_keeps_nulls() -> None:
+ """
+ A missing timestamp stays missing, and does not fail the report.
+
+ ``strftime`` on a ``NaT`` does not produce the string ``"NaT"``; it raises
+ ``ValueError: NaTType does not support strftime``, so without the
+ ``pd.notna`` guard one null would fail the whole report rather than print
+ anything.
+ """
+ df = pd.DataFrame.from_dict(
+ {"ds": {0: pd.Timestamp("2024-01-15"), 1: pd.NaT}},
+ )
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": "%Y"}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist()[0] == "2024"
+ assert pd.isna(formatted["ds"].tolist()[1])
+
+
[email protected](
+ "time_format",
+ [" %Y-%m-%d ", " %Y-%m-%d", "%Y-%m-%d ", "\t%Y-%m-%d\n",
"\r%Y-%m-%d\x0b"],
+)
+def test_table_time_format_strips_surrounding_whitespace(time_format: str) ->
None:
+ """
+ A padded format renders like the chart, which trims before formatting.
+
+ ``TimeFormatterRegistry.get`` trims the format string before building the
+ formatter, while d3 itself does not: ``utcFormat(" %Y ")`` returns
+ ``" 2024 "``. Without the strip the report kept the padding the chart had
+ already dropped. Every character here is whitespace to both ``str.strip``
+ and ``String.prototype.trim``; the handful they disagree on is in
+ ``format_time_column``.
+ """
+ df = pd.DataFrame.from_dict({"ds": {0: pd.Timestamp("2024-01-15")}})
+ form_data = {
+ "viz_type": "table",
+ "column_config": {"ds": {"d3TimeFormat": time_format}},
+ }
+
+ formatted = table(df, form_data)
+
+ assert formatted["ds"].tolist() == ["2024-01-15"]
+
+
[email protected](
+ "time_format",
+ [
+ "%Y Q%q",
+ "%H:%M:%S.%L",
+ "%Q",
+ "%-d %b",
+ "%x",
+ "local!%Y-%m-%d",
+ "%Y local!",
+ "%H:%M:%S.%f",
Review Comment:
Nothing in this rejection list exercises `%Z`, so adding `Z` to
`SHARED_TIME_DIRECTIVES` would keep every test green. After the UTC conversion
`strftime("%Y %Z")` renders `2024 UTC`, while d3's `%Z` renders `2024 +0000`,
so the report would stop matching the chart for any zone-bearing format. Could
`"%Y %Z"` be added here, asserting the timestamp comes back unchanged?
--
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]
