You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/08 17:15:39 UTC
[PR] feat(viz): bound list fields that fan out into extra queries [superset]
rusackas opened a new pull request, #40863:
URL: https://github.com/apache/superset/pull/40863
### SUMMARY
Two legacy viz code paths iterate a user-supplied list and issue one or more database queries per entry, with no bound on the list length:
- `NVD3TimeSeriesViz.run_extra_queries()` over `form_data['time_compare']`
- `DeckGLMultiLayer.get_data()` over `form_data['deck_slices']`
A single request with a large list therefore fans out into many queries. This adds two configurable upper bounds, checked **before** the loop runs:
| Config | Default |
|--------|---------|
| `VIZ_TIME_COMPARE_MAX_LIST_SIZE` | `10` |
| `VIZ_DECK_SLICES_MAX_LIST_SIZE` | `10` |
Set either to `0` to disable the bound. An over-limit list raises a clear `QueryObjectValidationError` instead of issuing the queries. The default of 10 is well above normal usage for these (deprecated) chart types.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend validation.
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/test_viz_get_df_payload.py
```
New tests: an over-limit `time_compare` and an over-limit `deck_slices` each raise; a `time_compare` within the limit proceeds normally.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4651789845
<!-- Bito Reply -->
The current implementation in `superset/viz.py` does not validate that `deck_slices` is a list, which could lead to issues if a non-list value is provided. Additionally, the configuration access for `VIZ_DECK_SLICES_MAX_LIST_SIZE` should use `.get(..., 0)` to prevent potential `KeyError` exceptions if the key is missing.
To address these, you should update the code to verify the type of `slice_ids` and use a safer configuration lookup:
```python
slice_ids = self.form_data.get("deck_slices")
if not isinstance(slice_ids, list):
slice_ids = []
max_deck_slices = current_app.config.get("VIZ_DECK_SLICES_MAX_LIST_SIZE", 0)
if max_deck_slices and len(slice_ids) > max_deck_slices:
# ... validation logic ...
```
**superset/viz.py**
```
slice_ids = self.form_data.get("deck_slices")
if not isinstance(slice_ids, list):
slice_ids = []
max_deck_slices = current_app.config.get("VIZ_DECK_SLICES_MAX_LIST_SIZE", 0)
if max_deck_slices and len(slice_ids) > max_deck_slices:
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #40863: feat(viz): bound list fields that fan out into extra queries
URL: https://github.com/apache/superset/pull/40863
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40863:
URL: https://github.com/apache/superset/pull/40863#discussion_r3375718579
##########
superset/viz.py:
##########
@@ -1664,7 +1673,15 @@ def get_data(self, df: pd.DataFrame) -> VizData:
from superset import db
from superset.models.slice import Slice
- slice_ids = self.form_data.get("deck_slices")
+ slice_ids = self.form_data.get("deck_slices") or []
+ max_deck_slices = current_app.config["VIZ_DECK_SLICES_MAX_LIST_SIZE"]
+ if max_deck_slices and len(slice_ids) > max_deck_slices:
Review Comment:
Good catch on the non-list case — added an `isinstance(slice_ids, list)` guard that coerces any non-list value to an empty list before we apply the size limit and `Slice.id.in_(...)`, so a stray string can no longer iterate characters or distort the count.
On `config.get(..., 0)`: `VIZ_DECK_SLICES_MAX_LIST_SIZE` ships with a default in `superset/config.py` (set to 10), so it is always present, and direct `config["..."]` access is the established convention throughout `viz.py`. Keeping that consistent here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4651738556
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40863?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `22.22222%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 54.98%. Comparing base ([`a183582`](https://app.codecov.io/gh/apache/superset/commit/a183582291ea020bae95bc10969c73c0470459d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fb484cc`](https://app.codecov.io/gh/apache/superset/commit/fb484cc9327b37c39af0e3ed838f8b6e3c0cbe00?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 343 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40863?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/viz.py](https://app.codecov.io/gh/apache/superset/pull/40863?src=pr&el=tree&filepath=superset%2Fviz.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdml6LnB5) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40863?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (a183582) and HEAD (fb484cc). Click for more details.
>
> <details><summary>HEAD has 150 uploads less than BASE</summary>
>
>| Flag | BASE (a183582) | HEAD (fb484cc) |
>|------|------|------|
>|python|138|1|
>|presto|14|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40863 +/- ##
==========================================
- Coverage 64.18% 54.98% -9.20%
==========================================
Files 2591 2637 +46
Lines 138471 142397 +3926
Branches 32120 32710 +590
==========================================
- Hits 88883 78303 -10580
- Misses 48056 63379 +15323
+ Partials 1532 715 -817
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.11% <22.22%> (+0.02%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.11% <22.22%> (-19.45%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40863/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/40863?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4659867437
@rusackas considering that this is required for the charts to load, should the default be `0` then? I imagine `10` could be a breaking change if a chart would need to go over it?
Also, is the DeckGL Multilayer fully deprecated?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #40863:
URL: https://github.com/apache/superset/pull/40863#discussion_r3375252573
##########
superset/viz.py:
##########
@@ -1664,7 +1673,15 @@ def get_data(self, df: pd.DataFrame) -> VizData:
from superset import db
from superset.models.slice import Slice
- slice_ids = self.form_data.get("deck_slices")
+ slice_ids = self.form_data.get("deck_slices") or []
+ max_deck_slices = current_app.config["VIZ_DECK_SLICES_MAX_LIST_SIZE"]
+ if max_deck_slices and len(slice_ids) > max_deck_slices:
Review Comment:
`deck_slices` is treated as an arbitrary sized iterable: if a client sends a non-list value (e.g. a string), `len(slice_ids)` will apply to the wrong thing and `Slice.id.in_(slice_ids)` will iterate characters, producing incorrect/empty results (or DB errors). Validate that `deck_slices` is a list before applying the size limit/query, and use `config.get(..., 0)` to avoid a hard KeyError if the config key is missing in a custom app config.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4651519333
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4651878366
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | fb484cc9327b37c39af0e3ed838f8b6e3c0cbe00 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a26f8bbaab82d00081fb76f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40863--superset-docs-preview.netlify.app](https://deploy-preview-40863--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcrLDYAgDADQXXqWiB6QuIFjtEKVhF8UTdC4u9ze4b2QjxRygRmgAzowrnszWyz97R5B6YpGYMjecXVxE96d5WzV2OxTXUzLCkfFmgiR9GiklHpgmhTD9wM) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40863--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4653992440
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(viz): bound list fields that fan out into extra queries [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40863:
URL: https://github.com/apache/superset/pull/40863#issuecomment-4662288026
Closing as superseded by #40636 (merged), which landed the same query-amplification bounds — `VIZ_TIME_COMPARE_MAX` / `DECK_MULTI_MAX_SLICES` in config.py, the matching bounds in `viz.py` for time_compare (`run_extra_queries`) and deck_slices (`DeckGLMultiLayer.get_data`), plus equivalent unit tests. Rebasing this branch onto master collapses it to a near-empty diff. #40636 also addresses @Vitor-Avila's review point about a too-low default — it shipped a more generous `50` (vs the `10` here) with clearer count-bearing error messages. If the small non-list `deck_slices` guard is worth keeping, I'll file it as a trivial standalone follow-up. Thanks @Vitor-Avila for the review!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org