bito-code-review[bot] commented on code in PR #44411:
URL: https://github.com/apache/superset/pull/44411#discussion_r4131705402
##########
superset/db_engine_specs/mysql.py:
##########
@@ -536,3 +539,136 @@ def cancel_query(cls, cursor: Any, query: Query,
cancel_query_id: str) -> bool:
return False
return True
+
+ @classmethod
+ def _requires_primary_key(cls, engine: Engine) -> bool:
+ """
+ Check whether the connected MySQL server rejects ``CREATE TABLE``
+ statements that do not declare a primary key
+ (``sql_require_primary_key = ON``, MySQL error 3750).
+
+ The variable was introduced in MySQL 8.0.13 and is absent from older
+ MySQL releases and from the MySQL-compatible engines that subclass
+ this spec (MariaDB, Doris, StarRocks, OceanBase), where querying it
+ errors out. Those servers do not enforce the requirement, so treat an
+ unreadable variable as "not required" rather than failing the upload.
+ """
+ try:
+ with engine.connect() as conn:
+ return bool(
+ conn.exec_driver_sql(
+ "SELECT @@session.sql_require_primary_key"
+ ).scalar()
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.debug(
+ "Unable to read @@session.sql_require_primary_key; "
+ "assuming a primary key is not required",
+ exc_info=True,
+ )
+ return False
+
+ @classmethod
+ def df_to_sql(
+ cls,
+ database: Database,
+ table: Table,
+ df: pd.DataFrame,
+ to_sql_kwargs: dict[str, Any],
+ ) -> None:
+ """
+ Upload a DataFrame to MySQL.
+
+ When the target table is being created and the server requires a
+ primary key (``sql_require_primary_key = ON``), the plain
+ ``pandas.DataFrame.to_sql`` call used by the base implementation
+ generates a ``CREATE TABLE`` without one, which MySQL rejects with
+ error 3750. Since MySQL rejects the bare ``CREATE TABLE`` itself,
+ the primary key has to be declared as part of that first statement;
+ it cannot be added afterwards with an ``ALTER TABLE``.
+
+ :param database: The database to upload the data to
+ :param table: The table to upload the data to
+ :param df: The dataframe with data to be uploaded
+ :param to_sql_kwargs: The kwargs to be passed to
+ pandas.DataFrame.to_sql
+ """
+ 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):
+ index = to_sql_kwargs.get("index", True)
+ index_label = to_sql_kwargs.get("index_label")
+ # A column promoted to PRIMARY KEY must reject duplicate and
+ # NULL values; the DataFrame index has neither guarantee (a
+ # CSV/Excel upload can point the "Dataframe index" option at
+ # a column that repeats or is missing values), so only
+ # promote it when it actually qualifies. Otherwise fall back
+ # to the synthesized key below, the same as the index=False
+ # path, and make sure the real index is not also written out
+ # as an extra column.
+ promote_index = (
+ bool(index) and df.index.is_unique and not df.index.hasnans
+ )
+ if promote_index:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>PK name collision guard missing</b></div>
<div id="fix">
The new fallback guards case-insensitive column collisions, but the promote
path does not: with `index_label`/`df.index.name` like "ID" and an existing
"id" column, `SQLTable.insert_data` reset_index only rejects exact
(case-sensitive) duplicates, so `CREATE TABLE` gets both names and MySQL fails
with error 1060. Extend `promote_index` with the same `existing_columns` check
the `else` branch uses.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Silent index drop without logging</b></div>
<div id="fix">
When the index has duplicates or NULLs, `promote_index` becomes False and
line 635 writes `index=False`, silently dropping the column the uploader's
"Dataframe index" option asked for. The old code failed loudly; now the
omission is invisible. Log a warning in the `else` fallback so users can tell
why the index column is missing.
</div>
</div>
<small><i>Code Review Run #04eed8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
