Copilot commented on code in PR #42821:
URL: https://github.com/apache/superset/pull/42821#discussion_r3726099060
##########
tests/unit_tests/db_engine_specs/test_mysql.py:
##########
@@ -87,6 +96,19 @@ def test_get_column_spec(
assert_column_spec(spec, native_type, sqla_type, attrs, generic_type,
is_dttm)
+def test_fetch_data_mutates_decimal_rows_in_tuple_results() -> None:
+ from superset.db_engine_specs.mysql import MySQLEngineSpec as spec #
noqa: N813
+
+ newdecimal, var_string = 246, 253
+ cursor = Mock()
+ cursor.description = [("amount", newdecimal), ("label", var_string)]
+ cursor.fetchall.return_value = (("10.50", "Ships"), ("22.30", "Planes"))
+
+ data = spec.fetch_data(cursor)
+
+ assert data == [(Decimal("10.50"), "Ships"), (Decimal("22.30"), "Planes")]
Review Comment:
This test currently depends on MySQLdb or pymysql being importable so
`get_datatype()` can translate the numeric type codes (246/253). Unit tests in
this file otherwise avoid requiring those optional DBAPI modules (see
`test_get_datatype_pymysql_fallback`), so this can become flaky depending on
the test environment. Consider stubbing `type_code_map` within the test to keep
it self-contained and restore it afterwards.
##########
superset/db_engine_specs/base.py:
##########
@@ -1339,6 +1339,7 @@ def fetch_data(cls, cursor: Any, limit: int | None =
None) -> list[tuple[Any, ..
)
}
if column_mutators:
+ data = list(data)
indexes = {row[0]: idx for idx, row in enumerate(description)}
Review Comment:
`data = list(data)` always creates a shallow copy when column mutators are
present, even when `fetchall()` already returns a mutable `list`. Since the
copy is only needed to handle immutable sequences (eg, mysqlclient returning a
tuple), this can be made conditional to avoid the extra O(n) copy for common
drivers.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -245,6 +245,43 @@ class MySQLEngineSpec(BasicParametersMixin,
BaseEngineSpec):
types.VARCHAR(),
GenericDataType.STRING,
),
+ # wire-protocol FIELD_TYPE names emitted by `get_datatype`, seen on
+ # SQL Lab and virtual dataset columns instead of DDL type names
+ (
+ re.compile(r"^newdecimal", re.IGNORECASE),
+ DECIMAL(),
+ GenericDataType.NUMERIC,
+ ),
+ (
+ re.compile(r"^tiny$", re.IGNORECASE),
+ TINYINT(),
+ GenericDataType.NUMERIC,
+ ),
+ (
+ re.compile(r"^short$", re.IGNORECASE),
+ types.SmallInteger(),
+ GenericDataType.NUMERIC,
+ ),
+ (
+ re.compile(r"^blob$", re.IGNORECASE),
+ types.String(),
+ GenericDataType.STRING,
+ ),
Review Comment:
The new `BLOB` mapping also looks like a good spot to cover the plain DDL
type `TEXT` (which otherwise doesn't match any existing `*text` patterns like
`tinytext`/`mediumtext`/`longtext`). Including `TEXT` here should help physical
dataset column type resolution without colliding with `tinytext` etc.
--
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]
