SEPURI-SAI-KRISHNA commented on code in PR #45433:
URL: https://github.com/apache/superset/pull/45433#discussion_r4371379745
##########
superset/utils/csv.py:
##########
@@ -115,6 +130,56 @@ 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):
+ # Every row label is one of its level's distinct values, so a frame
with
+ # nothing to escape is recognised from ``levels`` without a pass over
+ # the rows. The reverse does not hold: ``levels`` keeps values no row
+ # uses any more, which a filter or a slice leaves behind.
+ if any(needs_escaping(level) for level in index.levels):
+ # Only a level carrying a formula label among its row labels is
+ # rebuilt, and that is decided from the row labels rather than from
+ # ``levels``, because the rebuild costs a level its dtype:
+ # ``Index.map`` re-infers it, so a level of integers and nulls
comes
+ # back ``float64``, which writes an integer id as ``5.0`` and
rounds
+ # one past 2**53 to a different number. A stale ``levels`` entry
+ # would pay that for a label the export does not contain, while a
+ # level that does need rebuilding holds a string among its rows,
+ # which keeps the re-inferred dtype ``object``.
+ #
+ # A sibling keeps its own values and codes, and with them its
dtype.
+ # ``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. The row values are read 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.
Review Comment:
You are right, and I had the reason wrong rather than loosely worded.
`Index.map` runs position for position, so the original codes keep pointing
at
the right entries whatever the sort order. Checked with the hostile label
first, last and in the middle, including codes `[0, 2, 1]`, and the rows come
out correct in every one.
The gate agrees. With the rebuild switched to
`(index.codes[position], level.map(escape_values))`, the only failure in the
file is `escapes_colliding_index_labels`, on
`ValueError: Level values must be unique: ["'=a", "'=a"]`. The row-order test
passes under it, so it never separated the two.
Reworded both. The comment gives uniqueness as the reason and notes that
mapping `levels` would keep the rows right. The test docstring now says it
pins
placement rather than the choice of source, and points at the collision test
as
the case that separates them. The description had the same claim and is
corrected there too.
--
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]
