codeant-ai-for-open-source[bot] commented on PR #40005:
URL: https://github.com/apache/superset/pull/40005#issuecomment-4416939723
Question: -for-open-source review
Answer:
Thanks for the PR — the root cause and the intended fix make sense.
One thing I’d flag is a potential behavioral regression in the new
restoration logic:
- In `_restore_dropped_metric_columns()`, the MultiIndex path restores **all
combinations of the missing metric with every observed `columns` row** from
`orig_columns`.
- That means if a metric is dropped because it is all-NaN, it will be
reintroduced for every category combination, even when the original pivot
intentionally would have omitted some sparse combinations.
- The PR description says this is acceptable for “metric-level absences,”
but in practice this can make the output schema wider than before for charts
where a metric is absent only in some groups. I’d double-check this against
existing mixed-timeseries behavior to ensure it doesn’t bring back columns that
downstream code still expects to stay dropped.
A smaller implementation note:
- `df[key] = float("nan")` mutates the DataFrame column-by-column and relies
on pandas ordering semantics. That usually works, but if deterministic column
ordering is important for downstream rendering/tests, it may be worth
explicitly reindexing the columns after restoration rather than relying on
assignment order.
Overall, the fix is reasonable and the tests cover the intended null-metric
case well.
--
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]
