fix(mysql): resolve wire-protocol column types and mutate rows from immutable results#42203
fix(mysql): resolve wire-protocol column types and mutate rows from immutable results#42203massucattoj wants to merge 1 commit into
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #782d8aActionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| ) | ||
| } | ||
| if column_mutators: | ||
| data = list(data) |
There was a problem hiding this comment.
Suggestion: Unconditionally copying data to a new list when mutators are present duplicates the full result set in memory even when fetchall() already returned a mutable list. On large result sets this can cause avoidable memory spikes and potential worker OOMs; only copy when the returned container is immutable (for example, a tuple) so mutable list results can be mutated in place. [performance]
Severity Level: Major ⚠️
- ❌ Large Postgres queries may double memory, risking OOM.
- ⚠️ SQL Lab queries (`sql_lab.py:303`) duplicate result lists.
- ⚠️ Datasource introspection (`sqla/utils.py:162`) duplicates result containers.
- ⚠️ Chart data fetching (`models/core.py:865`) uses fetch_data.Steps of Reproduction ✅
1. Configure a Postgres database using psycopg2 and a table with an INTERVAL column;
Superset uses `PostgresEngineSpec.column_type_mutators` for INTERVAL (see
`superset/db_engine_specs/postgres.py:26-28`).
2. In SQL Lab, run a query returning many rows including that INTERVAL column; the
execution path calls `db_engine_spec.fetch_data(cursor, increased_limit)` at
`superset/sql_lab.py:303`.
3. Inside `BaseEngineSpec.fetch_data` (`superset/db_engine_specs/base.py:13-49`),
`cursor.fetchall()` returns a list of row tuples for psycopg2, and `column_mutators` is
built; because an INTERVAL column is present, `column_mutators` is non-empty and the
mutator branch is taken.
4. The mutator branch executes `data = list(data)` at
`superset/db_engine_specs/base.py:39-41`, creating a second full list copy even though
`data` is already a list; for large result sets this doubles in-memory row storage
temporarily, which can cause worker memory spikes or OOM failures when mutating rows.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 1239:1239
**Comment:**
*Performance: Unconditionally copying `data` to a new list when mutators are present duplicates the full result set in memory even when `fetchall()` already returned a mutable list. On large result sets this can cause avoidable memory spikes and potential worker OOMs; only copy when the returned container is immutable (for example, a tuple) so mutable list results can be mutated in place.
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
|
The flagged performance issue is correct. Unconditionally calling Here is the proposed fix for if column_mutators:
if not isinstance(data, list):
data = list(data)
indexes = {row[0]: idx for idx, row in enumerate(description)}This change ensures that if superset/db_engine_specs/base.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42203 +/- ##
=======================================
Coverage 65.07% 65.08%
=======================================
Files 2758 2758
Lines 155229 155230 +1
Branches 35572 35572
=======================================
+ Hits 101020 101029 +9
+ Misses 52294 52287 -7
+ Partials 1915 1914 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Columns of MySQL virtual datasets (created from SQL Lab queries) showed a "?" type icon in the chart panel instead of ABC/#, which also hid type-dependent controls such as X-Axis Sort By.
Physical datasets get column types from the SQLAlchemy inspector as DDL names (
VARCHAR(100),DECIMAL(15,2)), which matchMySQLEngineSpec.column_type_mappings. Virtual dataset columns instead get their type from the driver's cursor description viaget_datatype, which returns the raw wire-protocolFIELD_TYPEconstant names (VAR_STRINGfor VARCHAR,NEWDECIMALfor DECIMAL,TINYfor TINYINT,BLOBfor TEXT, and so on). None of those names matched any mapping regex, sotype_genericresolved toNone.This PR adds mappings for the 8 wire-protocol names that had no match:
VAR_STRING,NEWDECIMAL,TINY,SHORT,BLOB,YEAR,ENUMandSET. Regexes are anchored so they don't collide with existing DDL patterns (^tiny$vs^tinytext), and^enum\b/^set\balso cover the DDL forms (ENUM('a','b')), which were equally unresolved.Fixing the type resolution exposed a second, latent bug: MySQL registers a
column_type_mutatorsentry that converts DECIMAL string values to PythonDecimal, keyed on the resolved column type. It had never fired becauseNEWDECIMALnever resolved. Once it did,BaseEngineSpec.fetch_datacrashed withTypeError: 'tuple' object does not support item assignment, because the mutation loop assigns rows back into thefetchall()result and mysqlclient returns a tuple of rows (psycopg2 returns a list, which is why the same code works for Postgres). Fixed by copying the result into a list before mutating.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION