sadpandajoe commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r3979861188
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -244,23 +250,25 @@ export const useDownloadMenuItems = (
},
];
+ const xlsxExportLabel = (mode: 'data' | 'images', text: string) =>
+ exportingXlsx === mode ? t('Preparing export…') : text;
+
const exportMenuItems: MenuItem[] = [
...(userCanExport
? [
{
key: 'export-xlsx',
- label: t('Export Data to Excel'),
+ label: xlsxExportLabel('data', t('Export Data to Excel')),
+ disabled: exportingXlsx !== null,
onClick: () => onExportXlsx('data'),
},
- // Image export renders charts through the headless webdriver, so
only
- // offer it where that infrastructure is available (same signal as
the
- // PDF/PNG image downloads above); otherwise non-table charts would
- // silently come back empty.
+ // Image exports require the same webdriver flags as PDF and PNG.
...(isWebDriverScreenshotEnabled
Review Comment:
This still exposes `Export Images to Excel` whenever the webdriver flags are
enabled, but the server now unconditionally returns 400 for that mode when
`EXCEL_EXPORT_S3_BUCKET` is unset. In the newly supported no-storage
configuration the menu therefore offers an action that can never succeed and
tells an end user to change an operator setting. Could storage capability be
exposed to the client so this item is hidden or disabled when image export
cannot run?
##########
superset/dashboards/excel_export/sync_budget.py:
##########
@@ -0,0 +1,85 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Plan and size dashboard Excel exports served in the HTTP response."""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import Any
+
+from flask import current_app
+
+from superset.dashboards.excel_export.layout import get_charts_in_layout_order
+from superset.dashboards.excel_export.workbook import (
+ resolve_query_context,
+ ResolvedQueryContexts,
+)
+
+
+@dataclass(frozen=True)
+class InlineExportPlan:
+ """Queries planned for a direct download and their row budget."""
+
+ #: Resolved query contexts by chart id. ``None`` marks a skipped chart.
+ query_contexts: ResolvedQueryContexts
+ #: Combined row limit, or ``None`` when any query has no finite limit.
+ requested_rows: int | None
+ #: Configured limit for direct downloads.
+ max_rows: int
+
+ @property
+ def fits_row_budget(self) -> bool:
+ """Return whether the export can run during the request."""
+ return self.requested_rows is not None and self.requested_rows <=
self.max_rows
+
+
+def _finite_row_limit(query: Any) -> int | None:
+ """Return the limit; missing, null, and zero use ``ROW_LIMIT``."""
+ if not isinstance(query, dict):
+ return None
+ row_limit = query.get("row_limit") or current_app.config["ROW_LIMIT"]
Review Comment:
The default calculation rejects common small KPI dashboards: Big Number
charts expose no row-limit control and return a single aggregate row, but each
omitted limit is charged the default `ROW_LIMIT` of 50,000. With the new
100,000 sync ceiling, three such tiles get a whole-dashboard 400 and the advice
to lower chart limits is impossible to follow. Could aggregate queries be
budgeted by their bounded result shape, or could the defaults be calibrated so
this no-configuration path supports ordinary KPI dashboards?
##########
superset/dashboards/excel_export/sync_budget.py:
##########
@@ -0,0 +1,85 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Plan and size dashboard Excel exports served in the HTTP response."""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import Any
+
+from flask import current_app
+
+from superset.dashboards.excel_export.layout import get_charts_in_layout_order
+from superset.dashboards.excel_export.workbook import (
+ resolve_query_context,
+ ResolvedQueryContexts,
+)
+
+
+@dataclass(frozen=True)
+class InlineExportPlan:
+ """Queries planned for a direct download and their row budget."""
+
+ #: Resolved query contexts by chart id. ``None`` marks a skipped chart.
+ query_contexts: ResolvedQueryContexts
+ #: Combined row limit, or ``None`` when any query has no finite limit.
+ requested_rows: int | None
+ #: Configured limit for direct downloads.
+ max_rows: int
+
+ @property
+ def fits_row_budget(self) -> bool:
+ """Return whether the export can run during the request."""
+ return self.requested_rows is not None and self.requested_rows <=
self.max_rows
+
+
+def _finite_row_limit(query: Any) -> int | None:
+ """Return the limit; missing, null, and zero use ``ROW_LIMIT``."""
+ if not isinstance(query, dict):
+ return None
+ row_limit = query.get("row_limit") or current_app.config["ROW_LIMIT"]
+ if isinstance(row_limit, bool) or not isinstance(row_limit, int):
+ return None
+ return row_limit if row_limit > 0 else None
+
+
+def _row_total(query_contexts: ResolvedQueryContexts) -> int | None:
+ """Rows every resolved query may return, or ``None`` if any is
unbounded."""
+ total = 0
+ for query_context in query_contexts.values():
+ if query_context is None:
+ # Skipped charts do not add to the row budget.
+ continue
+ for query in query_context["queries"]:
+ row_limit = _finite_row_limit(query)
+ if row_limit is None:
+ return None
+ total += row_limit
+ return total
+
+
+def plan_inline_export(dashboard: Any) -> InlineExportPlan:
+ """Resolve a dashboard's queries and calculate its direct-download size."""
+ query_contexts: ResolvedQueryContexts = {
+ chart.id: resolve_query_context(chart)
Review Comment:
A single malformed legacy chart can now abort the entire direct export
before the workbook builder's per-chart isolation runs. `resolve_query_context`
can raise while rebuilding JSON-valid but schema-invalid params (for example, a
string-valued `columns` reaches list-only code); the queued path catches that
inside `build_workbook` and lists the chart as skipped, while this unguarded
comprehension turns the no-storage path into a 500 with no workbook. Could
planning preserve the same per-chart failure isolation, while still propagating
`SoftTimeLimitExceeded`?
##########
superset/dashboards/excel_export/sync_budget.py:
##########
@@ -0,0 +1,85 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Plan and size dashboard Excel exports served in the HTTP response."""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import Any
+
+from flask import current_app
+
+from superset.dashboards.excel_export.layout import get_charts_in_layout_order
+from superset.dashboards.excel_export.workbook import (
+ resolve_query_context,
+ ResolvedQueryContexts,
+)
+
+
+@dataclass(frozen=True)
+class InlineExportPlan:
+ """Queries planned for a direct download and their row budget."""
+
+ #: Resolved query contexts by chart id. ``None`` marks a skipped chart.
+ query_contexts: ResolvedQueryContexts
+ #: Combined row limit, or ``None`` when any query has no finite limit.
+ requested_rows: int | None
+ #: Configured limit for direct downloads.
+ max_rows: int
+
+ @property
+ def fits_row_budget(self) -> bool:
+ """Return whether the export can run during the request."""
+ return self.requested_rows is not None and self.requested_rows <=
self.max_rows
+
+
+def _finite_row_limit(query: Any) -> int | None:
+ """Return the limit; missing, null, and zero use ``ROW_LIMIT``."""
+ if not isinstance(query, dict):
+ return None
+ row_limit = query.get("row_limit") or current_app.config["ROW_LIMIT"]
+ if isinstance(row_limit, bool) or not isinstance(row_limit, int):
+ return None
+ return row_limit if row_limit > 0 else None
+
+
+def _row_total(query_contexts: ResolvedQueryContexts) -> int | None:
+ """Rows every resolved query may return, or ``None`` if any is
unbounded."""
+ total = 0
+ for query_context in query_contexts.values():
+ if query_context is None:
+ # Skipped charts do not add to the row budget.
+ continue
+ for query in query_context["queries"]:
+ row_limit = _finite_row_limit(query)
Review Comment:
This budget can approve work that is unbounded at execution time. On engines
without native grouping sets, `QueryContextProcessor._grouping_sets_fallback`
fans the query out once per level and explicitly sets every subquery's
`row_limit` to `None`, even though this planner counted the saved finite limit
only once. A dashboard can therefore pass the 100k guard and run several
unbounded queries in the web request. Could the planner reject or accurately
cost contexts whose execution can discard/expand the declared limit?
--
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]
