sadpandajoe commented on code in PR #45823:
URL: https://github.com/apache/superset/pull/45823#discussion_r4450616383
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -403,10 +403,9 @@ def pivot( # pylint: disable=too-many-arguments # noqa:
C901
if percent_mode is not None:
df = _apply_show_values_as(df, percent_mode)
- if combine_value_with_metric:
- # stack() no longer introduces rows of NA values to drop (pandas
>=2.1's
- # "future" stack implementation, the only one since pandas 3.0), so the
- # restored all-NaN metric rows are preserved without a dropna=
argument.
- df = df.stack(level=0).unstack()
+ if combine_value_with_metric and isinstance(df.columns, pd.MultiIndex):
+ df = df.reorder_levels([*range(1, df.columns.nlevels), 0],
axis=1).sort_index(
Review Comment:
This `sort_index(axis=1)` sorts every column level, whereas the old
`stack(level=0).unstack()` kept the order `pivot_table` produced. With
`marginal_distributions=True` and `combine_value_with_metric=True`, a
`pivot_table` result of `(m1,AU) (m1,US) (m1,All) (m2,AU) ...` used to come
back as `(AU,m1) (AU,m2) (US,m1) (US,m2) (All,m1) (All,m2)` and now comes back
as `(AU,m1) (AU,m2) (All,m1) (All,m2) (US,m1) (US,m2)`, so the totals column
lands in the middle instead of last.
`test_pivot_combine_preserves_column_pairs` sorts both frames before comparing,
so it can't see this. Can the reorder avoid sorting the non-metric levels?
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -986,32 +986,31 @@ def _convert_temporal_columns(df: pd.DataFrame, dtype:
dict[str, Any]) -> None:
for column_name, sqla_type in dtype.items():
if isinstance(sqla_type, (Date, DateTime)):
try:
- df[column_name] = pd.to_datetime(df[column_name])
- except OutOfBoundsDatetime:
- # Row-level fallback: coerce only OOB values; re-raise for
malformed
- # strings. Whole-column errors="coerce" would silently swallow
- # malformed values that happen to share a column with an OOB
date.
- original = df[column_name].copy()
- result = []
- for val in original:
- if pd.isna(val):
- result.append(pd.NaT)
- continue
- try:
- result.append(pd.to_datetime(val))
- except OutOfBoundsDatetime:
- result.append(pd.NaT)
- # Other exceptions (e.g. malformed strings) propagate
- converted = pd.Series(result, index=original.index)
- n_coerced = int(converted.isna().sum() - original.isna().sum())
- if n_coerced > 0:
- logger.warning(
- "Coerced %d out-of-bounds datetime value(s) "
- "in column '%s' to NaT",
- n_coerced,
- column_name,
- )
- df[column_name] = converted
+ converted = pd.to_datetime(df[column_name])
+ if pd.api.types.is_datetime64_any_dtype(converted):
+ df[column_name] = converted
+ continue
+ except ValueError:
Review Comment:
Wrapping `except ValueError` around the vectorized `pd.to_datetime` sends
every parse error into the per-row fallback, not just out-of-bounds ones.
pandas now infers one format from the first value and raises when a later value
doesn't match, so a column like `["01/02/2020", "05/03/2020", "13/02/2020"]`
used to fail the import with a format mismatch, but now imports as 2020-01-02,
2020-05-03, 2020-02-13: the first two month-first and the third day-first, with
no warning since the log only counts out-of-bounds NaTs. Could the fallback be
limited to `OutOfBoundsDatetime` and the "Mixed timezones" case, re-raising
other `ValueError`s?
--
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]
