codeant-ai-for-open-source[bot] commented on code in PR #40885:
URL: https://github.com/apache/superset/pull/40885#discussion_r3570966150
##########
superset/charts/client_processing.py:
##########
@@ -403,5 +410,11 @@ def apply_client_processing( # noqa: C901
index=show_default_index,
**current_app.config["CSV_EXPORT"],
)
+ elif query["result_format"] == ChartDataResultFormat.XLSX:
+ excel.apply_column_types(processed_df, query["coltypes"])
+ query["data"] = excel.df_to_excel(
+ processed_df,
+ **current_app.config["EXCEL_EXPORT"],
+ )
Review Comment:
**Suggestion:** The XLSX serialization path does not pass an explicit
`index` flag, so pandas defaults to writing the index. This diverges from the
chart export behavior that suppresses default `RangeIndex`, causing
post-processed report attachments to gain an unexpected extra index column.
Pass the same computed index policy used by the CSV branch (eg, based on
whether the index is default). [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β XLSX attachments include unintended default index column.
- β οΈ XLSX report tables differ from CSV formatting policy.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Construct a pandas DataFrame with a default `RangeIndex` (e.g.,
`pd.DataFrame({"city":
["Paris", "London"], "value": [10, 20]})`) and wrap it in a chart query
result for the
`table` viz type so that `post_processor` in `apply_client_processing`
returns a frame
with a `RangeIndex` (superset/charts/client_processing.py:283-305, 313-325).
2. Call `apply_client_processing(result, form_data)` and observe that
metadata computation
sets `show_default_index = not isinstance(processed_df.index,
pd.RangeIndex)` to `False`
for this frame (client_processing.py:379), meaning default `RangeIndex`
should not be
shown in exports.
3. For CSV result format, the code writes `query["data"] =
csv.df_to_escaped_csv(processed_df, index=show_default_index,
**current_app.config["CSV_EXPORT"])` (client_processing.py:405-412), passing
`index=False`
when the index is a `RangeIndex`, which omits the index as intended and
matches other
export paths like `query_context_processor`
(superset/common/query_context_processor.py:2-23).
4. For XLSX result format, the new branch at lines 413-418 calls
`excel.apply_column_types(processed_df, query["coltypes"])` and then
`query["data"] =
excel.df_to_excel(processed_df, **current_app.config["EXCEL_EXPORT"])`
without an explicit
`index` argument. With default `EXCEL_EXPORT` (superset/config.py:1436)
lacking an
`"index"` key, `df.to_excel` inside `df_to_excel`
(superset/utils/excel.py:14-22) defaults
to `index=True`, causing the emitted XLSX bytes to include the `RangeIndex`
as an extra
first column even though `show_default_index` was `False` and CSV exports
suppress it.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=47e44651449f4fbeaaa4e3c83777baba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=47e44651449f4fbeaaa4e3c83777baba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 413:418
**Comment:**
*Logic Error: The XLSX serialization path does not pass an explicit
`index` flag, so pandas defaults to writing the index. This diverges from the
chart export behavior that suppresses default `RangeIndex`, causing
post-processed report attachments to gain an unexpected extra index column.
Pass the same computed index policy used by the CSV branch (eg, based on
whether the index is default).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=667c0612dac607d6d4332268e1ad61b1a7f856732a2fad5c7a2131c0ff528131&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=667c0612dac607d6d4332268e1ad61b1a7f856732a2fad5c7a2131c0ff528131&reaction=dislike'>π</a>
##########
superset/charts/client_processing.py:
##########
@@ -357,6 +357,13 @@ def apply_client_processing( # noqa: C901
sep=sep,
decimal=decimal,
)
+ elif query["result_format"] == ChartDataResultFormat.XLSX:
+ read_excel_kwargs = (
+ {"index_col": 0}
+ if current_app.config["EXCEL_EXPORT"].get("index", True)
+ else {}
+ )
+ df = pd.read_excel(BytesIO(data), **read_excel_kwargs)
Review Comment:
**Suggestion:** The XLSX reader is deciding whether to consume an index
column from `EXCEL_EXPORT["index"]`, but exported chart data does not reliably
encode index presence from that config (it is based on the actual dataframe
index shape). With the default config (`EXCEL_EXPORT` empty), this forces
`index_col=0` and can drop the first real data column for files exported
without an index. Derive index handling from the payload characteristics (or
fallback-read logic), not from config alone. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β XLSX post-processing drops first column when index absent.
- β οΈ Email chart XLSX attachments show misaligned columns and index.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Start with the default Superset configuration where `EXCEL_EXPORT` is an
empty dict
(superset/config.py:1436), so
`current_app.config["EXCEL_EXPORT"].get("index", True)`
returns `True`.
2. In a test or runtime flow, construct an XLSX payload for a simple table
without an
index column by calling `excel.df_to_excel(source_df, index=False)` as in
`_assert_xlsx_client_processing`
(superset/tests/unit_tests/charts/test_client_processing.py:2-18), and build
`result =
{"queries": [{"result_format": ChartDataResultFormat.XLSX, "data":
xlsx_bytes}]}`.
3. Call `apply_client_processing(result, form_data)` for a table viz
(superset/charts/client_processing.py:313-325), which iterates queries and
reaches the
XLSX input branch at lines 360-366: `read_excel_kwargs = {"index_col": 0} if
current_app.config["EXCEL_EXPORT"].get("index", True) else {}; df =
pd.read_excel(BytesIO(data), **read_excel_kwargs)`.
4. Because the XLSX payload was written without an index column,
`pd.read_excel(...,
index_col=0)` in `apply_client_processing` (client_processing.py:360-366)
consumes the
first real data column as the index, dropping it from `df.columns`. The
subsequent
metadata (`colnames`, `indexnames`, `rowcount`) and any XLSX post-processed
exports built
from this frame will be based on a corrupted table where the first data
column has been
turned into the index.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b2a553bb15d4460b94188f677ed76581&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b2a553bb15d4460b94188f677ed76581&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 360:366
**Comment:**
*Logic Error: The XLSX reader is deciding whether to consume an index
column from `EXCEL_EXPORT["index"]`, but exported chart data does not reliably
encode index presence from that config (it is based on the actual dataframe
index shape). With the default config (`EXCEL_EXPORT` empty), this forces
`index_col=0` and can drop the first real data column for files exported
without an index. Derive index handling from the payload characteristics (or
fallback-read logic), not from config alone.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=e3ea4d002ee7846fa51ca302af748161c6135b9fa35218f23549e8e32f19c8e8&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=e3ea4d002ee7846fa51ca302af748161c6135b9fa35218f23549e8e32f19c8e8&reaction=dislike'>π</a>
##########
superset/commands/report/execute.py:
##########
@@ -897,6 +934,13 @@ def _get_notification_content(self) ->
NotificationContent: # noqa: C901
csv_data = self._get_csv_data()
if not csv_data:
error_text = "Unexpected missing csv file"
+ elif (
+ self._report_schedule.chart
+ and self._report_schedule.report_format ==
ReportDataFormat.XLSX
+ ):
+ xlsx_data = self._get_xlsx_data()
+ if not xlsx_data:
+ error_text = "Unexpected missing xlsx file"
Review Comment:
**Suggestion:** XLSX generation is wired only for chart-backed schedules,
but there is no corresponding hard validation to reject dashboard schedules
configured with XLSX. Since XLSX is now an accepted report format, a dashboard
schedule can pass through content generation without hitting any attachment
branch and send a misleading βsuccessfulβ email with no file. Add server-side
validation that restricts XLSX to chart targets (or raise an explicit execution
error when target is unsupported). [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Dashboard XLSX reports send emails without any attachments.
- β οΈ Users receive success notifications for unsupported XLSX dashboards.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Create or update a `ReportSchedule` via the reports API using
`ReportSchedulePostSchema` or `ReportSchedulePutSchema`, setting `dashboard`
to a valid
dashboard ID, leaving `chart` unset, and choosing `report_format="XLSX"`
(superset/reports/schemas.py:9-13, 173-177). The schema only validates that
`report_format` is one of `ReportDataFormat` values and does not restrict
formats based on
whether a chart or dashboard is targeted.
2. Persist this schedule; the `ReportSchedule` model stores `report_format`
as a string
defaulting to `ReportDataFormat.PNG` but now also allowing `XLSX`
(superset/reports/models.py:82-86, 120), with `dashboard_id` populated and
`chart_id` left
`NULL`.
3. Trigger execution of this dashboard schedule so
`BaseReportState._get_notification_content` runs
(superset/commands/report/execute.py:903-123). In the attachment routing
block
(execute.py:919-985), PNG and PDF formats are handled unconditionally, CSV
and XLSX
formats are only handled when `self._report_schedule.chart` is truthy
(execute.py:931-941), and there is no branch for a dashboard with
`ReportDataFormat.XLSX`.
For the dashboard-only schedule, the XLSX branch at lines 937-943 is
skipped, leaving
`xlsx_data` as `None` and `error_text` unset.
4. `_get_notification_content` falls through to build a
`NotificationContent` with a name
derived from the dashboard title and `xlsx=None` (execute.py:102-121). The
email
notification plugin attaches XLSX data only if `content.xlsx` is set
(superset/reports/notifications/email.py:106-118), so the schedule sends a
"successful"
dashboard email that advertises XLSX format in
`header_data["notification_format"]` but
contains no XLSX attachment and no explicit error explaining that dashboards
do not
support XLSX.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf2d002ee0724a449f3c4ab450f86d14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bf2d002ee0724a449f3c4ab450f86d14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 937:943
**Comment:**
*Incomplete Implementation: XLSX generation is wired only for
chart-backed schedules, but there is no corresponding hard validation to reject
dashboard schedules configured with XLSX. Since XLSX is now an accepted report
format, a dashboard schedule can pass through content generation without
hitting any attachment branch and send a misleading βsuccessfulβ email with no
file. Add server-side validation that restricts XLSX to chart targets (or raise
an explicit execution error when target is unsupported).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=8c672e32cf3911b9bc49f007d8bffcb9ffcde319db2e3b964912967e05e7ed81&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=8c672e32cf3911b9bc49f007d8bffcb9ffcde319db2e3b964912967e05e7ed81&reaction=dislike'>π</a>
--
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]
