SEPURI-SAI-KRISHNA commented on code in PR #45433:
URL: https://github.com/apache/superset/pull/45433#discussion_r4367857847
##########
superset/utils/csv.py:
##########
@@ -115,6 +130,48 @@ 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):
+ # Only a level carrying a formula label is rebuilt. A sibling keeps
+ # its own values and codes, and with them its dtype: read back row
+ # by row it would lose a missing value to ``float64``, which writes
+ # an integer id as ``5.0`` and rounds one past 2**53 to a different
+ # number. ``escape_value`` prepends a quote, so two distinct labels
+ # can escape to the same text, and ``factorize`` folds those onto a
+ # single code, which the level values of a ``MultiIndex`` have to
+ # be unique for. It reads the level's row values rather than
+ # ``levels``, which is sorted rather than in row order, so reusing
+ # the original codes against escaped ``levels`` would put each
+ # label on the wrong row as soon as a hostile one did not sort
+ # first.
+ rebuilt = [
+
pd.factorize(index.get_level_values(position).map(escape_values))
+ if needs_escaping(level)
Review Comment:
Confirmed, and your repro is exact: sliced to its last two rows it exported
`9007199254740992.0`, with nothing in the output that needed escaping.
The mechanism is `Index.map` re-inferring the dtype. `[9007199254740993,
None]`
comes back `float64`, while `['=a', 9007199254740993, None]` stays object
because the string holds it there. That makes the fix stricter than it
looks: a
level that does need rebuilding always has a string among its rows, so the
corruption only ever happened on a rebuild that was not needed. Deciding from
the row values removes it rather than narrowing it.
The decision now reads `index.get_level_values(position)`, as you suggested.
I
kept `levels` for the guard in front of the block, since every row label is
one
of its level's distinct values, so as a negative check it cannot miss one,
and
it costs 0.011 ms against 58.6 ms over the row values on a 200k-row frame
with
50 and 20 distinct labels. No test separates that guard; it is an
optimisation.
My description was wrong next door, and is corrected. It listed the per-level
decision as separable by no input because forcing a rebuild "produces an
identical index". Reverting it fails both sibling-dtype tests and the new
case,
so it was separable all along, and that belief is what made reading the
decision
off `levels` look harmless.
New test:
`test_df_to_escaped_csv_keeps_the_dtype_of_a_level_whose_label_was_sliced_out`.
--
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]
