SEPURI-SAI-KRISHNA commented on code in PR #45433:
URL: https://github.com/apache/superset/pull/45433#discussion_r4343723182
##########
superset/utils/csv.py:
##########
@@ -115,6 +130,32 @@ def escape_values(v: Any) -> Union[str, Any]:
if isinstance(value, str):
df.at[label, name] = escape_value(value)
+ # Row labels and axis names are written out whenever the caller asks for
the
+ # index, which both export paths do once post-processing has replaced the
+ # range index: pivot_table_v2 promotes the row dimension's warehouse values
+ # into index labels and the dimension name into ``index.name``. Escaping
+ # happens after the cell loop so the labels ``.at`` addresses stay the ones
+ # the frame was handed over with. Every level of a MultiIndex is covered;
+ # the axis names are a separate attribute, hence the ``rename_axis``. This
+ # mirrors the four places ``superset.utils.excel.quote_formulas`` covers
+ # for the xlsx writer.
+ index = df.index
+ if isinstance(index, pd.MultiIndex):
+ # ``levels`` carries each level's distinct values, so an index with
+ # nothing to escape is recognised without a pass over every row.
+ if any(needs_escaping(level) for level in index.levels):
+ index = pd.MultiIndex.from_arrays(
+ [
+ escape_axis_level(index.get_level_values(position))
Review Comment:
Confirmed, and it is worse than reformatting: the id changes. With
`MultiIndex.from_tuples([("=a", 9007199254740993), ("safe", None)],
names=["dim", "id"])` master writes `9007199254740993` and the branch writes
`9007199254740992.0`, so the export carries an id the warehouse never returned.
Your `5` / `5.0` case is the same coercion one row earlier. It only bites when
the sibling level holds a missing value, since `get_level_values` on a complete
integer level stays `int64`, which is why the existing sibling test did not
catch it.
One note on the suggested shape.
`index.levels[position].take(index.codes[position])` codes a missing value as
`-1`, and `take(-1)` wraps to the last entry, so on `[("=a", 5), ("b", None),
("c", 99)]` the id level comes back as `[5, 99, 99]`: the missing id becomes a
real id belonging to another row. `allow_fill=True` does not rescue it either,
it raises `ValueError: Unable to fill values because Index cannot contain NA`
on an `int64` level.
So I took your parenthetical instead and narrowed the rebuild to the level
that needs it. Each level keeps its own values and codes unless it carries a
formula label, and a level that does is rebuilt by factorizing its escaped row
values, which is also what keeps the colliding-label case working:
`escape_value` prepends a quote, so two distinct labels can escape to the same
text, and `MultiIndex` requires unique level values.
Measured against master on every case in the suite, the only two outputs
that change are the ones you named, and both now match master exactly:
```
master before after
int sibling + null =a,5,1 '=a,5.0,1 '=a,5,1
big int sibling =a,9007199254740993,1 '=a,9007199254740992.0,1
'=a,9007199254740993,1
```
The colliding-label, `float32`, `datetime64`,
missing-value-in-a-hostile-level and nothing-to-escape cases are byte identical
before and after.
Pinned by
`test_df_to_escaped_csv_keeps_a_sibling_level_dtype_with_a_missing_value`,
which uses the large id so the assertion fails on the rounding and not just on
the decimal point. Reverting to the previous rebuild fails it.
--
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]
