fix(csv-import): add primary key when required by MySQL #37547
Conversation
dosubot
bot
added
data:connect:mysql
![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 #528e4b
Actionable Suggestions - 2
-
superset/db_engine_specs/mysql.py - 1
- Unclosed parenthesis in conditional expression · Line 456-458
-
tests/integration_tests/databases/commands/upload_mysql_pk_test.py - 1
- Test assertion too permissive · Line 236-236
Review Details
-
Files reviewed - 2 · Commit Range:
16de1f8..16de1f8- superset/db_engine_specs/mysql.py
- tests/integration_tests/databases/commands/upload_mysql_pk_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
| needs_primary_key = ( | ||
| if_exists == "fail" # Only for new table creation | ||
| and not to_sql_kwargs.get("index", False) # No index column exists |
There was a problem hiding this comment.
Line 456 has an unclosed opening parenthesis ( in the needs_primary_key assignment. The condition started on line 456 is missing a closing parenthesis before the assignment completes on line 458.
Code suggestion
Check the AI-generated fix before applying
| needs_primary_key = ( | |
| if_exists == "fail" # Only for new table creation | |
| and not to_sql_kwargs.get("index", False) # No index column exists | |
| needs_primary_key = ( | |
| if_exists == "fail" # Only for new table creation | |
| and not to_sql_kwargs.get("index", False) # No index column exists | |
| ) |
Code Review Run #528e4b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if len(execute_calls) >= 2: | ||
| alter_call = execute_calls[1][0][0] | ||
| # The column name should be modified to avoid conflict | ||
| assert "__superset_upload_id__" in alter_call or "___superset_upload_id__" in alter_call |
There was a problem hiding this comment.
The assertion allows either the original or prefixed column name, but the code always prefixes when there's a conflict, so the test should verify the exact adjusted name.
Code suggestion
Check the AI-generated fix before applying
- assert "___superset_upload_id__" in alter_call
+ assert "___superset_upload_id__" in alter_call and "__superset_upload_id__" not in alter_call
Code Review Run #528e4b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
Thanks for the review! This change only affects backend CSV import logic. |
1 participant
SUMMARY
Fixes CSV import failures on MySQL databases that enforce primary keys
(e.g. when
sql_require_primary_key = ON).When importing a CSV, Superset generates a CREATE TABLE statement without
a primary key. Databases that require primary keys correctly reject this
operation. This change detects when a primary key is required and
automatically adds a safe auto-increment primary key during table creation,
while preserving existing behavior for databases that do not enforce this
constraint.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
TESTING INSTRUCTIONS
Automated testing:
Manual testing (optional):
sql_require_primary_key = ONprimary key
without this requirement
ADDITIONAL INFORMATION