SEPURI-SAI-KRISHNA commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4403744256
##########
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:
Confirmed. Adding `Z` keeps all 315 green, and they diverge both ways: after
the UTC conversion `strftime("%Y %Z")` is `2024 UTC` against d3's `2024
+0000`, and on a naive value `strftime` writes nothing where d3 still writes
`+0000`.
The gap was coverage rather than one letter, so I ran every letter through
both and pinned each one they disagree on that was not already covered: `%z
%c %X %s %C %D %F %h %k %l %n %P %r %R %t %T`. Most are ones `strftime`
substitutes while d3 copies the bare letter, so `%T` would read `13:45:09`
against a literal `T` in the chart. `%s` is the odd one: d3 reads the epoch
in
UTC and glibc in local time, so the two sit a server offset apart.
All 22 rejected directives now fail on their own when their letter is added
to
the set. The seven outside the set that do agree are left unpinned on
purpose,
so widening stays possible.
--
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]
