SEPURI-SAI-KRISHNA commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4333967110
##########
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:
You are right, and the guard is load-bearing: a string column carrying
`d3TimeFormat` reaches `.strftime` and raises `AttributeError` in the worker.
Added `test_table_skips_a_time_format_on_a_non_datetime_column`, parametrized
over a string column and an integer one, asserting both come back unchanged.
Deleting the `is_datetime64_any_dtype` line fails it.
Running that same check over the rest of the diff found a second gate with no
test behind it, so it is in the same commit. `is_strftime_expressible`
requires
at least one `%` directive, and nothing pinned that. Without it a format like
`yyyy-mm-dd` or `Date` is accepted, `strftime` finds nothing to substitute
and
hands the string back as it is, and every cell in the column reads
`yyyy-mm-dd`. `test_table_skips_a_time_format_with_no_directive` covers it.
That rule is also what rejects `smart_date`, which means the separate
`time_format != SMART_DATE` comparison in `table()` could never fire. I
dropped
it and moved the explanation next to the rule that does the work, so
`test_table_leaves_adaptive_time_format_alone` now pins the real reason.
I also re-ran the directive set against the real `d3-time-format` rather than
reading it, over seven dates covering a leap day, both year boundaries and an
ISO week that belongs to the previous year. Every directive in
`SHARED_TIME_DIRECTIVES` matches `strftime` on all of them. The ones that
agree
on those dates but are still excluded are there for the reasons in the
comment:
`%a %A %b %B %p` agree only because the test ran in English with no
`D3_TIME_FORMAT` set, and `%f` agrees only up to millisecond precision, since
d3 scales what it has (`123456` microseconds in, `123000` out).
For the rest of the diff, every added condition now has a named test that
fails
when it alone is reverted, except two that no input can separate: the
`continue`
after `format_time_column` (the column is text by then, so
`is_numeric_dtype` is
already false and no later branch can act on it) and the `coltype ==
TEMPORAL`
gate in `payload_coltypes` (`extract_dataframe_dtypes` only departs from the
values when `is_dttm` is set, so the override would be a no-op everywhere it
newly applied). Both are in the description as kept for what they say rather
than for what they do, in case you would rather they go.
--
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]
