SEPURI-SAI-KRISHNA commented on code in PR #45433:
URL: https://github.com/apache/superset/pull/45433#discussion_r4334734680
##########
superset/utils/csv.py:
##########
@@ -115,6 +115,18 @@ 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. ``rename`` applies the mapper to every
+ # level of a MultiIndex; 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.
+ df = df.rename(index=escape_values)
Review Comment:
Confirmed, and the numbers are yours exactly. On pandas 2.3.3, with the
frames
from your comment:
| index | master | this branch before the change |
| --- | --- | --- |
| `float32` labels `1.1`, `2.2` | `1.1`, `2.2` | `1.100000023841858`,
`2.200000047683716` |
| `datetime64[s]` year 1500, `date_format="%d/%m/%Y"` | `01/01/1500` |
`1500-01-01 00:00:00` |
The widening is `rename` rebuilding the level: `float32` goes to `float64`
and
the wider dtype prints the binary expansion of the stored value, and the
datetime index comes back as object dtype, which `to_csv` does not apply
`date_format` to.
It also reaches a MultiIndex, though not uniformly: a `float32` level
survives
`rename`, but a `datetime64[s]` level still lands as object, so a pivot on a
dimension and a time grain wrote `1500-01-01 00:00:00` where the configured
format asked for `01/01/1500`.
I went with your suggestion, but keyed on the values rather than the dtype.
Levels are now only rebuilt when they actually hold a label `escape_value`
would change:
```python
def needs_escaping(level: pd.Index) -> bool:
return any(
isinstance(value, str) and escape_value(value) != value for
value in level
)
def escape_axis_level(level: pd.Index) -> pd.Index:
return level.map(escape_values) if needs_escaping(level) else level
```
Two reasons for not gating on the dtype. `pd.api.types.is_string_dtype` is
True
for any object dtype, mixed contents included, so it does not separate the
levels that need escaping from the ones that must keep their dtype. And a
`CategoricalIndex` of strings reports False, so a dtype gate would have
stopped
escaping labels the current code does escape.
A MultiIndex goes through `MultiIndex.from_arrays`, which preserves each
level's
dtype and tolerates two labels escaping to the same value (`=a` and `'=a`
both
become `'=a`), where `set_levels` raises on that. The decision is read off
`index.levels`, the distinct values, so an index with nothing to escape is
recognised without a pass over every row.
Three tests, each failing on the previous commit with the values above:
`test_df_to_escaped_csv_keeps_a_numeric_index_dtype`,
`test_df_to_escaped_csv_keeps_a_temporal_index_dtype` and
`test_df_to_escaped_csv_keeps_a_sibling_level_dtype` for the MultiIndex case.
The collision is now covered too, since nothing pinned it:
`test_df_to_escaped_csv_escapes_colliding_index_labels` puts `=a` and `'=a`
in
one level and asserts the file, and swapping `from_arrays` back for
`set_levels` fails it with `ValueError: Level values must be unique`.
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -278,6 +278,63 @@ def test_df_to_escaped_csv_non_default_index():
assert rows[2] == ["girl Mary", "safe"]
+def test_df_to_escaped_csv_escapes_index_labels():
+ """
+ Formulas in row labels are escaped too, not just cells and headers.
+
+ Pivot post-processing promotes the row dimension's values into the index,
+ and both export paths write the index out once it is no longer a
+ RangeIndex, so a hostile warehouse string can reach the first column of
+ the file.
+ """
+ df = pd.DataFrame(
+ data={"SUM(num)": [10.0, 20.0]},
+ index=pd.Index(['=cmd|" /C calc"!A0', "safe"], name="dim"),
+ )
+
+ rows = df_to_escaped_csv(df, encoding="utf8",
index=True).strip().split("\n")
+
+ assert rows[1].startswith("\"'=cmd")
+ assert rows[2].startswith("safe,")
+ # The caller's frame is left alone.
+ assert df.index.tolist() == ['=cmd|" /C calc"!A0', "safe"]
+
+
+def test_df_to_escaped_csv_escapes_multiindex_labels_and_axis_names():
+ """
+ Every level of a MultiIndex is escaped, and so are the axis *names*.
+
+ Axis names are a separate attribute from the labels, so ``rename`` alone
+ leaves them untouched while pandas still writes them out as header cells.
+ A pivoted group-by puts a warehouse-controlled column name there.
+ """
+ df = pd.DataFrame(
+ data={"v": [1, 2]},
+ index=pd.MultiIndex.from_tuples(
+ [("a", '=cmd|" /C calc"!A0'), ("b", "safe")],
+ names=["level", "+name"],
+ ),
+ )
+
+ rows = df_to_escaped_csv(df, encoding="utf8",
index=True).strip().split("\n")
+
+ # Header row carries the escaped axis name.
+ assert rows[0] == "level,'+name,v"
Review Comment:
You are right, and I checked it the way you described: dropping
`columns=escape_values` from the `rename_axis` call leaves
`test_df_to_escaped_csv_escapes_multiindex_labels_and_axis_names` green, and
the
pivot you gave writes its first two lines as `=axis,outer` and `+inner,v`,
raw.
Added `test_df_to_escaped_csv_escapes_column_axis_names` with your frame:
```python
df = pd.DataFrame(
data=[[1]],
index=pd.Index(["row"], name="dim"),
columns=pd.MultiIndex.from_tuples([("outer", "v")], names=["=axis",
"+inner"]),
)
rows = df_to_escaped_csv(df, encoding="utf8",
index=True).strip().split("\n")
assert rows[0] == "'=axis,outer"
assert rows[1] == "'+inner,v"
```
It fails with the mapper removed and passes with it, which is the pin that
was
missing.
--
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]
