fix(mysql): handle string typed decimal results

[villebro]

SUMMARY

MySQL returns DECIMAL typed values as strings, rather than Decimal or float. This causes issues in the frontend, as most plugins expect GenericDataType.NUMERIC values to be of type number, not string. To get around this we introduce a new property column_type_mutators on BaseEngineSpec, where type-specific callbacks can be introduced to mutate the results coming back from the database. Here we implement logic on the MySQL spec to change string-based decimals to Decimal, similar to what Postgres already does automatically (I assume most connectors do the same).

The new logic can be expensive for big datasets, due to the need to iterate over the full result set. For this reason the for-loop is only triggered if there are columns present that need to be mutated.

AFTER

Now the Big Number chart works as expected on Trino Decimal types (notice the value is now number in the response payload):

BEFORE

Previously Big Number chart would display "No data" due to the data type being string:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue: closes EXPORT_CSV decimal separator format doesn't work for Decimal MySQL column #18218
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #24241 (043668c) into master (50535e4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 043668c differs from pull request most recent head 261baf4. Consider uploading reports for the commit 261baf4 to get more accurate results

@@           Coverage Diff           @@
##           master   #24241   +/-   ##
=======================================
  Coverage   68.30%   68.31%           
=======================================
  Files        1957     1957           
  Lines       75594    75611   +17     
  Branches     8224     8224           
=======================================
+ Hits        51637    51654   +17     
  Misses      21849    21849           
  Partials     2108     2108           

Flag	Coverage Δ
hive	53.37% <64.28%> (+<0.01%)	⬆️
mysql	78.91% <75.00%> (-0.01%)	⬇️
postgres	78.98% <75.00%> (-0.01%)	⬇️
presto	53.29% <64.28%> (+<0.01%)	⬆️
python	82.82% <100.00%> (+<0.01%)	⬆️
sqlite	77.52% <75.00%> (-0.01%)	⬇️
unit	53.44% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/base.py	90.96% <100.00%> (+0.15%)	⬆️
superset/db_engine_specs/mysql.py	98.85% <100.00%> (+0.02%)	⬆️
superset/db_engine_specs/trino.py	84.12% <100.00%> (+0.38%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[villebro]

Notice how we're changing from list[list[Any]] to list[tuple[Any, ...]] here - this is because this is what SQLA drivers should return. This will save us from having to do the conversion later here:

superset/superset/result_set.py

Lines 132 to 134 in 50535e4

	# only do expensive recasting if datatype is not standard list of tuples
	if data and (not isinstance(data, list) or not isinstance(data[0], tuple)):
	data = [tuple(row) for row in data]

[kgabryje]

Can we add a comment that explains the logic here? This dict comprehension might be hard to understand without the context of this PR

[kgabryje]

LGTM + a small nitpick about adding comments

[zhaoyongjie]

Typically decimal is a fixed point data structure but native float in Python is not fixed point data, so some drivers return a string to represent fixed point. column_type_mutators adds some extra transformation on the decimal column so the query performance might become lower than before.

From the screenshot before and after, I saw the coltypes field changed from 1 -> 0, do we have some other approach which that just change the coltypes instead to mutate entire values of column?

[villebro]

Good catch about the coltypes, let me investigate why that's gone missing.

Wrt to the query performance hit, another option is to make the frontend more resilient to receiving various types. In this case we could cast the value to number in the frontend if the coltype is GenericDataType.NUMERIC. However, I'd prefer to do this type of transformation in the backend to ensure that the plugins don't have to deal with all the nuances that SQLAlchemy throws our way.

I'd be curious to hear other peoples thoughts, @michael-s-molina @john-bodley any thoughts here?

[villebro]

It hit me, that without this change, the linked issue won't be fixed, as CSV export happens purely in the backend. With this new pattern we could even normalize non-standard timestamps early in the flow, eliminating the need for doing it in the frontend, further decoupling the quirks of the analytical databases from the downstream logic.

[villebro]

Btw, I just noticed we already have logic like this in the codebase:

superset/superset/db_engine_specs/ocient.py

Lines 330 to 334 in 50535e4

	# Peek at the schema to determine which column values, if any,
	# require sanitization.
	columns_to_sanitize: List[PlacedSanitizeFunc] = _find_columns_to_sanitize(
	cursor
	)

[john-bodley]

@villebro for Trino the DECIMAL type is no longer experimental per trinodb/trino-python-client#308, i.e., now by default these types use the native Python type.

Furthermore, regarding MySQL, doesn't the handling of the DECIMAL depend on how the various dialects handle this?

[villebro]

@villebro for Trino the DECIMAL type is no longer experimental per trinodb/trino-python-client#308, i.e., now by default these types use the native Python type.

Oh nice, thanks for the heads up. Let me confirm that by bumping the version 👍 I notice the original PR was changing other return types, too, have we confirmed that Superset handles them well?

Furthermore, regarding MySQL, doesn't the handling of the DECIMAL depend on how the various dialects handle this?

That may well be - let me check with the author of the issue which dialect they were using.

[john-bodley]

@villebro in #24069 I bumped the version of python-trino-client to the latest version somewhat recently, i.e., within the past couple of weeks.

[villebro]

@villebro in #24069 I bumped the version of python-trino-client to the latest version somewhat recently, i.e., within the past couple of weeks.

Oh nice, I didn't know it had this many improvements. Let me also check if those pesky non-standard timestamp strings are now replaced with proper datetimes that I had to work around in #23339

[john-bodley]

@villebro do you plan on resurrecting this PR?

[villebro]

@john-bodley I do, if there's still apetite for it. But as far as Trino is concerned, this doesn't seem needed anymore. So not sure if we want to introduce this additional complexity if it's not really needed.

[villebro]

FYI @john-bodley I removed the Trino logic as it's now redundant, but for MySQL this does seem to be needed. I'm going to follow-up with the author of the Ocient connector to see if we can migrate their custom mutator logic to this more standardized model.