fix(bigquery): limit result set size to prevent browser memory crashes - #38588
Conversation
Implement memory-aware progressive fetching in BigQuery's fetch_data method. Large result sets (950+ MB) previously crashed Chrome by loading everything into memory at once. The fix samples an initial batch to estimate row size, then fetches only as many rows as fit within the BQ_FETCH_MAX_MB config limit (default 200 MB). A warning toast is shown to users when results are truncated. This is always-on with no feature flag -- operators control the budget via the BQ_FETCH_MAX_MB config constant. Originally by @ethan-l-geotab in #36387. Co-authored-by: ethan-l-geotab <ethanliong@geotab.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #e5147f
Actionable Suggestions - 3
-
superset/db_engine_specs/bigquery.py - 2
- Inaccurate memory estimation · Line 335-335
- Avoid blind exception catch · Line 367-367
-
superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts - 1
- Backend schema missing warning field · Line 80-80
Review Details
-
Files reviewed - 7 · Commit Range:
1773531..1773531- superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts
- superset-frontend/src/components/Chart/chartAction.ts
- superset/common/query_context_processor.py
- superset/config.py
- superset/db_engine_specs/bigquery.py
- tests/unit_tests/common/test_query_context_processor.py
- tests/unit_tests/db_engine_specs/test_bigquery.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
codeant-ai-for-open-source
Bot
added
the
size:L
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Implements a memory-aware progressive fetch in the BigQuery engine spec to prevent large result sets from overwhelming browser memory, and propagates a truncation warning through the backend response so the frontend can notify the user.
Changes:
- Add progressive, memory-budgeted fetching in
BigQueryEngineSpec.fetch_data(default cap via newBQ_FETCH_MAX_MBconfig). - Propagate a truncation warning into chart data responses and show it as a frontend warning toast.
- Add unit tests for BigQuery fetch behavior and warning propagation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
superset/db_engine_specs/bigquery.py |
Adds progressive fetch logic and sets truncation flags for warning propagation. |
superset/common/query_context_processor.py |
Adds a warning field to the per-query payload when truncation is detected. |
superset/config.py |
Introduces BQ_FETCH_MAX_MB default configuration value (200 MB). |
superset-frontend/src/components/Chart/chartAction.ts |
Displays a warning toast when a query response includes warning. |
superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts |
Extends query response typing to include optional warning. |
tests/unit_tests/db_engine_specs/test_bigquery.py |
Adds unit tests for the new BigQuery fetch/truncation behavior and fallback. |
tests/unit_tests/common/test_query_context_processor.py |
Adds unit tests ensuring warning is included/omitted appropriately. |
You can also share your feedback on Copilot code review. Take the survey.
- Use has_app_context()/has_request_context() guards so fetch_data is safe to call outside a Flask request (fixes RuntimeError on g writes and current_app access in non-request paths) - Replace sys.getsizeof(str(batch)) with per-row getsizeof sum for more accurate memory estimation without the str() allocation - Fix false-positive truncation: fetch remaining+1 rows and check len > remaining to confirm more data exists beyond the cap - Reset g.bq_memory_limited/g.bq_memory_limited_row_count after reading in get_df_payload to prevent flag leaking across multiple queries in the same request - Wrap warning string in _() for i18n - Add warning field to ChartDataResponseResult Marshmallow schema - Pass noDuplicate: true to addWarningToast to suppress duplicate toasts when a multi-query chart has multiple truncated responses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38588 +/- ##
==========================================
- Coverage 64.19% 64.17% -0.02%
==========================================
Files 2592 2592
Lines 139080 139161 +81
Branches 32299 32316 +17
==========================================
+ Hits 89281 89313 +32
- Misses 48267 48313 +46
- Partials 1532 1535 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Cache persistence: persist bq_memory_limited flag in QueryCacheManager so truncation warnings survive cache hits. The flag is now saved into the cache value dict in set_query_result (reading from g and resetting it there), and restored from cache in get(). get_df_payload now reads cache.bq_memory_limited instead of g, removing the need for g entirely in query_context_processor.py. Frontend test: add two tests to chartActions.test.ts verifying that addWarningToast is dispatched (with noDuplicate: true) when a query response carries a warning field, and not dispatched when it doesn't. Updated existing test_query_context_processor tests to set the flag on mock_cache directly rather than patching g. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #97d812Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sys.getsizeof(row) on a tuple only measures the container, not the referenced cell values. Switch to summing container + element sizes (one level deep) for a more accurate memory budget estimate. Most BigQuery cell values are primitives (str, int, float, date), so one level captures the dominant allocation without deep recursion overhead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #8a5284Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
The new bq_memory_limited warning path in get_df_payload iterates all cache mocks in existing tests. Those pre-existing tests don't set bq_memory_limited on their MagicMock/MockCache instances, causing TypeErrors on f-string format or AttributeErrors. Set the field to False/0 in the three impacted tests so they exercise the no-warning code path.
Code Review Agent Run #6a86fbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
LGTM — well-crafted fix for a real production pain point. The progressive-fetch approach with an upfront sample is sound, and the author has already addressed all bot-review concerns (false-positive truncation detection, g-flag leakage, cache persistence, LocalProxy safety, i18n wrapping, frontend test coverage).
One MEDIUM style nit worth fixing before merge:
superset/db_engine_specs/bigquery.py:317—current_app.config.get("BQ_FETCH_MAX_MB", 200)should becurrent_app.config["BQ_FETCH_MAX_MB"]since the key has a default inconfig.py. The Superset convention isconfig["KEY"]—.get()with a fallback hides missing-config bugs and duplicates the default unnecessarily.
Non-blocking NITs:
bigquery.py:322— magic number1000for the initial sample batch size could be a named constant (_BQ_INITIAL_SAMPLE_ROWS) for clarity/testability.bigquery.py:379— the broadexcept Exception(already pylint-disabled) would benefit from a comment naming the expected failure modes (BQ DB-API errors, network timeouts, unexpected Row subtypes).test_query_context_processor.py:1615, 1675— the two new test functions are missing-> Nonereturn type annotations.
Particularly well done: the +1 fetch trick to confirm truncation without false positives, the g-flag cache persistence pattern, and the noDuplicate: true toast dedup.
- bigquery.py: switch `config.get("BQ_FETCH_MAX_MB", 200)` to bracket
access in-context. The default lives in config.py; bracket access
surfaces a missing key as a loud KeyError rather than silently
masking it with a duplicated fallback. The 200 still applies in the
no-app-context branch.
- bigquery.py: extract `1000` initial sample size to module-level
`_BQ_INITIAL_SAMPLE_ROWS` constant with a docstring.
- bigquery.py: add a comment to the broad `except Exception` naming
the expected failure modes (DB-API errors, network/auth timeouts,
`sys.getsizeof` on unknown cell types, future `Row` subclasses).
- test_query_context_processor.py: add `-> None` return annotations
to the two new `test_get_df_payload_bq_memory_limited_*` functions.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the careful review @aminghadersohi! Applied all four in 03521a4:
Pre-commit (ruff/mypy/pylint) all pass locally. Should be green and ready to merge once CI re-runs. |
|
Bito Automatic Review Skipped – PR Already Merged |
#38588) Co-authored-by: Claude Code <noreply@anthropic.com> Co-authored-by: ethan-l-geotab <ethanliong@geotab.com>
User description
SUMMARY
Adopts and improves the fix from #36387 (originally by @ethan-l-geotab). Fixes #36385.
BigQuery queries returning huge result sets (950+ MB) crash Chrome by loading everything into browser memory at once. This PR implements memory-aware progressive fetching in
BigQueryEngineSpec.fetch_data:BQ_FETCH_MAX_MBconfig limit (default 200 MB)gto the query context processor, which adds it to the response payloadBaseEngineSpec.fetch_dataimplementationKey differences from the original PR (#36387):
BQ_MEMORY_LIMIT_FETCHfeature flag -- the fix is always-onBQ_FETCH_MAX_MBconfig constant (default 200 MB) is the only operator-level knobchartAction.ts(was.jsin the original PR)Files changed:
superset/db_engine_specs/bigquery.py-- Memory-aware progressive fetch implementationsuperset/common/query_context_processor.py-- Warning propagation via Flaskgsuperset/config.py--BQ_FETCH_MAX_MB = 200config constantsuperset-frontend/src/components/Chart/chartAction.ts-- Warning toast displaysuperset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts--warningfield onChartDataResponseResulttests/unit_tests/db_engine_specs/test_bigquery.py-- 5 new test casestests/unit_tests/common/test_query_context_processor.py-- 2 new test casesBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Chrome crashes or becomes unresponsive when BigQuery returns 950+ MB of data.
After: Results are truncated to fit within the configured memory budget, and a warning toast informs the user.
TESTING INSTRUCTIONS
BQ_FETCH_MAX_MBto a smaller value (e.g., 10) insuperset_config.pyto test truncation with smaller datasetspytest tests/unit_tests/db_engine_specs/test_bigquery.py -k test_fetch_data -vADDITIONAL INFORMATION
Co-authored-by: ethan-l-geotab ethanliong@geotab.com
CodeAnt-AI Description
Limit BigQuery fetch size to avoid browser memory crashes and show truncation warning
What Changed
Impact
✅ Fewer browser crashes when querying BigQuery✅ Clearer warnings when chart data is truncated✅ Predictable memory usage during large BigQuery queries💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.