sadpandajoe commented on code in PR #45410:
URL: https://github.com/apache/superset/pull/45410#discussion_r4336546033
##########
superset/charts/client_processing.py:
##########
@@ -73,6 +75,32 @@
# The Excel equivalent, applied as a cell format so the value stays numeric.
EXCEL_PERCENT_FORMAT = "0.0%"
+# ``strftime`` copies a directive it does not recognize into its output
+# verbatim, so a d3-only one would reach the report as literal text: ``%L`` and
+# ``%q`` stay ``%L`` and ``%q`` rather than becoming milliseconds and a
quarter.
+# These are the directives that carry the same meaning in d3-time-format and in
+# ``strftime``. Left out are:
+#
+# * the ones that name a weekday, a month or a period (``%a``, ``%A``, ``%b``,
+# ``%B``, ``%p``). The browser reads those names from the d3 locale
+# definition, which ``D3_TIME_FORMAT`` overrides key by key, while
+# ``strftime`` reads them from the worker's ``LC_TIME``. The two agree only
+# when nothing is configured and the worker runs in English.
+# * the locale-composite ones (``%c``, ``%x``, ``%X``), whose output follows
+# the server's locale rather than the chart's.
+# * ``%f``. d3 derives it from ``getUTCMilliseconds() * 1000``, so the
browser's
+# microseconds always end in ``000``, while a warehouse column with
sub-second
+# precision reaches ``strftime`` with the real value.
+# * the platform-dependent or d3-only ones (``%e``, ``%g``, ``%L``, ``%q``,
+# ``%Q``, ``%s``, ``%Z``, ``%z``).
+SHARED_TIME_DIRECTIVES = frozenset("dGHIjmMSuUVwWyY%")
Review Comment:
Nothing in the suite pins most of this allowlist. The successful-format
tests only use `%Y`, `%d`, `%m`, `%H` and `%M`, so removing `%V` (or `%G`,
`%j`, `%U`, ...) from `SHARED_TIME_DIRECTIVES` would leave every test green
while silently turning ISO-week formats back into raw timestamps, and the d3
parity run described in the PR was manual. Could a parametrized `table()` case
assert literal output for the remaining directives, e.g. `2021-01-01` with
`%G-W%V-%u` giving `2020-W53-5`, plus a `%%` case?
##########
superset/charts/client_processing.py:
##########
@@ -835,6 +863,45 @@ def union_currency_context(
}
+def is_strftime_expressible(d3_time_format: str) -> bool:
+ """Whether every directive in a d3 time format means the same to
strftime."""
+ if LOCAL_TIME_PREFIX in d3_time_format:
+ # ``TimeFormatterRegistry.get`` strips the marker before building the
+ # formatter, so leaving it in would render it as literal text.
+ return False
+ directives = re.findall(r"%(.)", d3_time_format)
Review Comment:
A format with a stray trailing `%` passes this check: `re.findall(r"%(.)",
...)` only sees a `%` that has a character after it, so `%Y%` yields just `Y`.
`strftime` then copies the trailing `%` (`2024%` for `2024-01-15`) while d3
drops it (`2024`), so the report no longer matches the chart for that format.
Should a `%` that isn't consumed as part of a directive make the format
non-expressible?
##########
superset/charts/client_processing.py:
##########
@@ -1304,6 +1391,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 positional `iloc` read is what keeps repeated column labels lined up
with `extract_dataframe_dtypes`, but no test pins it. If this became
`df[df.columns[index]]`, a frame with two `ds` columns (one holding
`"02/01/2024"`, the other a real timestamp) would type both as `STRING`, and
the real timestamp column would lose `TEMPORAL` in the payload. Could a direct
`payload_coltypes` case with two columns labelled `ds` assert `[STRING,
TEMPORAL]`?
--
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]
