fix(csv-import): add primary key when MySQL requires one - #44411
sadpandajoe wants to merge 2 commits into
Conversation
Uploading a CSV/Excel/columnar file creates the target table via pandas.DataFrame.to_sql, which never declares a primary key. A MySQL server configured with sql_require_primary_key = ON rejects that CREATE TABLE with error 3750, so the upload fails outright. Build the table with an explicit primary key when the server requires one: the pandas index column if it is being written, otherwise a synthesized auto-numbered column. The key has to be part of the initial CREATE TABLE since MySQL rejects the bare statement itself. The sql_require_primary_key variable only exists in MySQL 8.0.13+, so probing it is best-effort -- older servers and the MySQL-compatible engines that subclass this spec (MariaDB, Doris, StarRocks, OceanBase) error on the query and fall back to the unchanged base implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44411 +/- ##
==========================================
+ Coverage 76.67% 80.48% +3.80%
==========================================
Files 2935 2933 -2
Lines 174904 174796 -108
Branches 40609 40576 -33
==========================================
+ Hits 134110 140677 +6567
+ Misses 38029 31450 -6579
+ Partials 2765 2669 -96
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:
|
![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 #543d2f
Actionable Suggestions - 2
-
tests/unit_tests/db_engine_specs/test_mysql.py - 2
- Inline imports violate BITO 12745 · Line 528-534
- Untyped Mock locals BITO 13153 · Line 571-571
Additional Suggestions - 2
-
tests/unit_tests/db_engine_specs/test_mysql.py - 2
-
Missing Any import NameError · Line 540-540`_enforce_sql_require_primary_key` annotates `conn: Any` and `parameters: Any` (lines 540, 543), but `Any` is not imported in `tests/unit_tests/db_engine_specs/test_mysql.py` — the module header imports only `datetime`, `Decimal`, `SimpleNamespace`, `Optional`, `Mock`, `patch`, etc. The annotation is evaluated at function-definition time, so collecting this file raises `NameError: name 'Any' is not defined` and every test in the module errors. Add `Any` to the typing imports.
-
Line exceeds 88-char lint limit · Line 578-578Line 578 is 97 characters, over Superset's 88-char limit configured in `pyproject.toml` (`flake8`/`RUF` section), so the repo's lint CI will fail on this changed line. The sibling query at lines 622-624 already wraps `conn.execute(sa.text(...))` across lines — apply the same wrapping here.
-
Review Details
-
Files reviewed - 2 · Commit Range:
b7d5740..0b7e0fb- superset/db_engine_specs/mysql.py
- tests/unit_tests/db_engine_specs/test_mysql.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- 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
| import pandas as pd | ||
| import sqlalchemy as sa | ||
| from sqlalchemy import create_engine, event | ||
| from sqlalchemy.exc import OperationalError | ||
|
|
||
| from superset.db_engine_specs.mysql import MySQLEngineSpec | ||
| from superset.sql.parse import Table |
There was a problem hiding this comment.
The three new tests each repeat the same inline imports (pandas, sqlalchemy, create_engine, MySQLEngineSpec/MariaDBEngineSpec, Table) inside the function bodies (lines 528-534, 597-602, 642-647). None of these modules is circular with this test file — sibling tests in this repo import pandas/sqlalchemy at module level — so per BITO rule 12745 these belong in the module header. This also removes the triplicated import blocks.
Code Review Run #543d2f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| mock_get_engine.return_value.__exit__.return_value = False | ||
|
|
||
| MySQLEngineSpec.df_to_sql( | ||
| database=Mock(), |
There was a problem hiding this comment.
The database=Mock() arguments at lines 571, 615, and 657 bind unannotated local-free literals; per BITO rule 13153, mock variables/dataset values in test files must carry explicit type annotations (e.g. database: Mock = Mock()). The sibling mock_get_engine bindings are also unannotated. This matches the rule's stated enforcement during code review.
Code Review Run #543d2f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness and compatibility issues remain in primary-key detection and key-generation behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes MySQL file uploads when sql_require_primary_key is enabled by creating upload tables with a primary key.
Changes:
- Adds MySQL primary-key requirement detection and custom table creation.
- Uses the DataFrame index or a synthesized key.
- Adds unit tests for required and unsupported server settings.
File summaries
| File | Summary |
|---|---|
tests/unit_tests/db_engine_specs/test_mysql.py |
Tests primary-key creation and fallback behavior. |
superset/db_engine_specs/mysql.py |
Implements MySQL-specific primary-key handling for uploads. |
Review details
Suppressed comments (4)
superset/db_engine_specs/mysql.py:606
- A newly created table with the default RangeIndex gets primary-key values 0..n-1 here. A later upload with
already_exists=appendanddataframe_index=Truewrites a fresh 0..n-1 index, so the unchanged append path will hit duplicate-primary-key errors. The generated key strategy needs to remain append-safe (for example, a dedicated auto-increment key or an explicit uniqueness/offset policy) and should be covered by an append regression test.
primary_key = index_label or df.index.name or "index"
superset/db_engine_specs/mysql.py:602
- The engine context is entered even for
if_exists='append', then this method delegates tosuper().df_to_sql, which opensget_engineagain. That means every append creates and tears down an unnecessary second engine context (including SSH/OAuth/prequery setup), contrary to the unchanged append path described in the PR. Delegate append before opening this context.
with cls.get_engine(
database,
catalog=table.catalog,
schema=table.schema,
) as engine:
creating_table = to_sql_kwargs.get("if_exists", "fail") != "append"
if creating_table and cls._requires_primary_key(engine):
superset/db_engine_specs/mysql.py:606
- On MySQL, a single integer primary-key column is implicitly
AUTO_INCREMENT. The default upload index starts at 0, so inserting[0, 1, 2]into this key (withNO_AUTO_VALUE_ON_ZEROoff) can turn 0 into the next generated value and then collide with the explicit 1 in the same multi-row insert. The SQLite test cannot exercise this dialect behavior; avoid auto-increment semantics for a user-provided index key or use a compatible key strategy, and add MySQL-specific coverage.
primary_key = index_label or df.index.name or "index"
superset/db_engine_specs/mysql.py:623
- When
keysis supplied, pandasSQLTablenames thePrimaryKeyConstraintas<table_name>_pk. The upload schema allows long names, and a 64-character table name is valid in MySQL, but the resulting 67-character constraint name exceeds MySQL's 64-character identifier limit, so this path rejects otherwise valid uploads. Use a bounded constraint name or avoid pandas' hard-coded constraint name.
keys=[primary_key],
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| with engine.connect() as conn: | ||
| return bool( | ||
| conn.exec_driver_sql( | ||
| "SELECT @@session.sql_require_primary_key" |
| index = to_sql_kwargs.get("index", True) | ||
| index_label = to_sql_kwargs.get("index_label") | ||
| if index: | ||
| primary_key = index_label or df.index.name or "index" |
| primary_key = "id" | ||
| while primary_key in df.columns: | ||
| primary_key = f"_{primary_key}" |
SUMMARY
Uploading a CSV, Excel, or columnar file creates the target table via
pandas.DataFrame.to_sql, which never declares a primary key. A MySQL serverconfigured with
sql_require_primary_key = ONrejects thatCREATE TABLEoutright with error 3750, so the upload fails with no way for the user to
work around it — the key has to be part of the initial
CREATE TABLE, sinceMySQL rejects the bare statement itself and there is no table left to
ALTERafterwards.MySQLEngineSpecnow overridesdf_to_sql. When a table is being created andthe server requires a primary key, it builds the table through pandas'
SQLTablehelper with an explicit key:already being written is promoted to the primary key (this is the literal
case in Unable to import CSV into hosted database that enforces primary key ID #37399 — the reporter's
CREATE TABLEhad that column but it wasnever marked
PRIMARY KEY);colliding with an existing column.
sql_require_primary_keyonly exists in MySQL 8.0.13+, and severalMySQL-compatible specs subclass this one (MariaDB, Doris, StarRocks,
OceanBase, Aurora). Those servers error on the probe query and do not enforce
the requirement, so an unreadable variable is treated as "not required" and
the upload falls through to the unchanged base implementation. Rows are
inserted with the same multi-row
INSERTthe base implementation requests ondialects that support it, so the new path carries no throughput penalty.
Appends to an existing table are untouched, and no behaviour changes on a
server with
sql_require_primary_key = OFF(the default).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only, no UI change.
TESTING INSTRUCTIONS
Unit tests:
pytest tests/unit_tests/db_engine_specs/test_mysql.pyThree tests cover the behaviour. No live MySQL is required: an in-memory
SQLite engine stands in, with a SQLAlchemy event listener reproducing MySQL's
documented enforcement by rejecting any
CREATE TABLEthat lacks a primarykey. The tests verify that the table is created and populated, that the
pandas index column becomes the primary key rather than a redundant extra
column being added, and that a server which does not expose
sql_require_primary_keystill uploads successfully.Manual verification against a real server:
sql_require_primary_key = ON.enabled.
enabled and once without.
(3750, "Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set.").SHOW CREATE TABLE <your_table>— the table should have aPRIMARY KEYon the index column (index enabled) or on the synthesized column
(index disabled).
unaffected.
ADDITIONAL INFORMATION