sadpandajoe commented on code in PR #45410: URL: https://github.com/apache/superset/pull/45410#discussion_r4321248725
########## superset/charts/client_processing.py: ########## @@ -835,6 +852,26 @@ 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.""" + directives = re.findall(r"%(.)", d3_time_format) + return bool(directives) and all( + directive in SHARED_TIME_DIRECTIVES for directive in directives + ) + + +def format_time_column(df: pd.DataFrame, column: Any, d3_time_format: str) -> None: + """Format a temporal column in place with an explicit d3 time format. + + The caller gates this on ``is_strftime_expressible``, so every directive in + ``d3_time_format`` means the same thing here as it does in the browser. Null + values are left as-is rather than rendered as the string ``"NaT"``. + """ + df[column] = df[column].apply( + lambda value: value.strftime(d3_time_format) if pd.notna(value) else value Review Comment: For a timezone-aware column, `strftime` formats in the value's own offset, whereas the chart's default formatter (`utcFormat`) formats in UTC. With `%Y-%m-%d %H:%M`, `2024-01-01 00:30+05:30` renders as `2024-01-01 00:30` in the report but `2023-12-31 19:00` in the chart. Should aware values be converted to UTC before formatting so the report matches what the chart displays? ########## superset/charts/client_processing.py: ########## @@ -835,6 +852,26 @@ 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.""" + directives = re.findall(r"%(.)", d3_time_format) Review Comment: A `local!`-prefixed format (the prefix `TimeFormatterRegistry` strips before choosing local-time formatting) passes this check, since `%Y` etc. are all in the allowlist, and then goes to `strftime` unchanged. `d3TimeFormat: "local!%Y-%m-%d"` would render as the literal text `local!2024-01-15` in the report, while the chart shows `2024-01-15`. Could the prefix be rejected here, so the column is left raw like the other unsupported formats? -- 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]
