aminghadersohi commented on code in PR #45782:
URL: https://github.com/apache/superset/pull/45782#discussion_r4434201583
##########
superset/utils/csv.py:
##########
@@ -102,18 +103,31 @@ def escape_values(v: Any) -> Union[str, Any]:
# Escape csv headers
df = df.rename(columns=escape_values)
- # Escape csv values. Iterate by index label (via ``items``) rather than by
- # positional offset so the escaped value is written back to the correct row
- # even when the DataFrame has a non-default index (e.g. the flattened
- # MultiIndex produced by pivot_table_v2 post-processing). Pairing
positional
- # indices with the label-based ``.at`` accessor would otherwise create
- # phantom rows and corrupt the output. Only string cells are reassigned, so
- # the dtype of mixed object columns (e.g. nullable integers) is preserved.
- for name, column in df.items():
+ # Escape by physical position: row labels and verbose column names can
+ # repeat after pivot/flatten. Label-based assignment would overwrite every
+ # matching cell. Keep numeric dtypes for pandas; explicitly format numbers
+ # only in object columns, matching streaming CSV exports.
+ decimal_separator = kwargs.get("decimal", ".")
+ for column_position, (_, column) in enumerate(df.items()):
if pd.api.types.is_string_dtype(column.dtype):
- for label, value in column.items():
+ for row_position, value in enumerate(column):
if isinstance(value, str):
- df.at[label, name] = escape_value(value)
+ df.iat[row_position, column_position] = escape_value(value)
+ elif isinstance(value, Decimal) and value.is_finite():
+ # Preserve precision and scale without passing numeric text
+ # through the string formula-prefix escaping path.
+ df.iat[row_position, column_position] = format_decimal(
Review Comment:
Nit: Each finite Decimal is written back with its own `df.iat` call, so
chart and query-context CSVs of Decimal columns (untouched before) run several
times slower: 200k cells go from 0.46s to 1.67s. Building each column's values
and assigning once with `df.isetitem` keeps positions and output.
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -489,3 +490,97 @@ def test_get_chart_csv_data_closes_response(
== b"csv"
)
response.close.assert_called_once()
+
+
[email protected]("decimal_separator", [".", ","])
[email protected]("amount", ["12.3400", "-1.25", "0E-18"])
+def test_csv_export_honors_decimal_separator_for_exact_values(
+ decimal_separator: str, amount: str
+) -> None:
+ """Typed decimals keep their scale and match the float column's
separator."""
+ frame = pd.DataFrame(
+ {"amount": [Decimal(amount)], "float": [1.25], "text": ["keep.this"]}
+ )
+ expected_amount = format(Decimal(amount), "f").replace(".",
decimal_separator)
+ expected_float = "1.25".replace(".", decimal_separator)
+ result = df_to_escaped_csv(frame, index=False, sep=";",
decimal=decimal_separator)
+ assert (
+ result ==
f"amount;float;text\n{expected_amount};{expected_float};keep.this\n"
+ )
+
+
[email protected]("decimal_separator", [".", ","])
+def test_csv_export_honors_decimal_separator_for_mixed_numbers(
+ decimal_separator: str,
+) -> None:
+ """Mixed numeric cells match streaming exports without localizing
strings."""
+ frame = pd.DataFrame(
+ {"mixed": [Decimal("-12.3400"), 1.25, True, "1.25", "=formula", None]},
+ dtype=object,
+ )
+ amount = "-12.3400".replace(".", decimal_separator)
+ number = "1.25".replace(".", decimal_separator)
+ assert df_to_escaped_csv(
+ frame, index=False, decimal=decimal_separator, sep=";"
+ ) == (f'mixed\n{amount}\n{number}\nTrue\n1.25\n\'=formula\n""\n')
+
+
[email protected]("decimal_separator", [".", ","])
[email protected](
+ "duplicate_axis", ["unique", "index", "columns", "both", "multiindex"]
+)
+def test_csv_export_preserves_duplicate_labels(
+ decimal_separator: str, duplicate_axis: str
+) -> None:
+ """Formatting a cell must not overwrite another row or column with its
label."""
+ frame = pd.DataFrame(
+ [
+ [Decimal("1.2500"), Decimal("3.7500")],
+ [Decimal("2.5000"), Decimal("4.2500")],
+ ],
+ columns=["amount", "amount"]
+ if duplicate_axis in {"columns", "both"}
+ else ["first", "second"],
+ index=["group", "group"]
+ if duplicate_axis in {"index", "both"}
+ else ["first", "second"],
+ )
+ if duplicate_axis == "multiindex":
+ frame.index = pd.MultiIndex.from_tuples([("group", 1), ("group", 1)])
+ original = frame.copy(deep=True)
+ expected_values = "1.2500;3.7500\n2.5000;4.2500\n".replace(".",
decimal_separator)
+ expected_header = ";".join(frame.columns)
+
+ assert (
+ df_to_escaped_csv(frame, index=False, sep=";",
decimal=decimal_separator)
+ == f"{expected_header}\n{expected_values}"
+ )
+ pd.testing.assert_frame_equal(frame, original)
+
+
[email protected]("decimal_separator", [".", ","])
+def test_csv_export_duplicate_labels_preserve_mixed_types(
+ decimal_separator: str,
+) -> None:
+ """Duplicate labels preserve exact numbers, nulls, bools and formula
escaping."""
+ frame = pd.DataFrame(
+ [
+ [Decimal("-1.2500"), 1.5, True, 7, "=1+2"],
+ [Decimal("2.5000"), None, False, 8, "keep.this"],
Review Comment:
Nit: Every null in these tests is `None`, which is not `Real`, so nothing
reaches the `pd.notna` guard: dropping it writes `nan` for a float NaN in an
object column with `decimal=","` and the suite still passes. A NaN here pins it.
```suggestion
[Decimal("2.5000"), np.nan, False, 8, "keep.this"],
```
##########
superset/utils/csv.py:
##########
@@ -107,13 +108,27 @@ def escape_values(v: Any) -> Union[str, Any]:
# even when the DataFrame has a non-default index (e.g. the flattened
# MultiIndex produced by pivot_table_v2 post-processing). Pairing
positional
# indices with the label-based ``.at`` accessor would otherwise create
- # phantom rows and corrupt the output. Only string cells are reassigned, so
- # the dtype of mixed object columns (e.g. nullable integers) is preserved.
+ # phantom rows and corrupt the output. Numeric dtypes are left for pandas;
+ # object columns need explicit formatting to match streaming CSV exports.
+ decimal_separator = kwargs.get("decimal", ".")
for name, column in df.items():
if pd.api.types.is_string_dtype(column.dtype):
for label, value in column.items():
if isinstance(value, str):
df.at[label, name] = escape_value(value)
+ elif isinstance(value, Decimal) and value.is_finite():
+ # Preserve precision and scale without passing numeric text
+ # through the string formula-prefix escaping path.
+ df.at[label, name] = format_decimal(value).replace(
+ ".", decimal_separator
+ )
Review Comment:
Confirmed at 6904e72f43: positional `iat` writes keep `1.2500`/`2.5000` and
`1.25`/`2.50` distinct with repeated row labels, repeated column names and
both, for Decimal and object-float cells.
`test_csv_export_preserves_duplicate_labels` fails on 2e375d528a.
--
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]
