You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2023/05/29 09:28:17 UTC
[GitHub] [superset] villebro opened a new pull request, #24241: fix(trino,mysql): handle string typed decimal results
villebro opened a new pull request, #24241:
URL: https://github.com/apache/superset/pull/24241
### SUMMARY
Trino and MySQL return `DECIMAL` typed values as strings, rather than `Decimal` or `float`. This is This causes issues in the frontend, as most plugins expect `GenericDataType.NUMERIC` values to be of type `number`, not `string`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
- [x] Has associated issue: closes #18218
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1571791555
> @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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209846315
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
Btw, I just noticed we already have logic like this in the codebase: https://github.com/apache/superset/blob/50535e427e51743e298d46009d51b08ef2380012/superset/db_engine_specs/ocient.py#L330-L334
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1568879890
@villebro in https://github.com/apache/superset/pull/24069 I bumped the version of `python-trino-client` to the latest version somewhat recently, i.e., within the past couple of weeks.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209699364
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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 text based timestamps in the backend, eliminating the need for doing it in the frontend, further decoupling the quirks of the analytical databases from the downstream logic.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1660757826
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24241: fix(mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1741260668
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #24241: fix(mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #24241:
URL: https://github.com/apache/superset/pull/24241
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209338846
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
Review Comment:
Can we add a comment that explains the logic here? This dict comprehension might be hard to understand without the context of this PR
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209397283
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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 just change the `coltypes` instead to mutate entire values of column?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1568852483
> @villebro for Trino the `DECIMAL` type is no longer experimental per [trinodb/trino-python-client#308](https://github.com/trinodb/trino-python-client/pull/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](https://docs.sqlalchemy.org/en/20/dialects/mysql.html) handle this?
That may well be - let me check with the author of the issue which dialect they were using.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1660741207
@villebro do you plan on resurrecting this PR?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209156539
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -366,3 +369,37 @@ def test_handle_cursor_early_cancel(
assert cancel_query_mock.call_args[1]["cancel_query_id"] == query_id
else:
assert cancel_query_mock.call_args is None
+
+
+@pytest.mark.parametrize(
+ "data,description,expected_result",
+ [
+ (
+ [["1.23456", "abc"]],
+ [("dec", "decimal(12,6)"), ("str", "varchar(3)")],
+ [(Decimal("1.23456"), "abc")],
Review Comment:
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: https://github.com/apache/superset/blob/50535e427e51743e298d46009d51b08ef2380012/superset/result_set.py#L132-L134
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1566854764
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24241](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (eaf4923) into [master](https://app.codecov.io/gh/apache/superset/commit/50535e427e51743e298d46009d51b08ef2380012?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (50535e4) will **decrease** coverage by `11.21%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head eaf4923 differs from pull request most recent head f5a2f73. Consider uploading reports for the commit f5a2f73 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24241 +/- ##
===========================================
- Coverage 68.30% 57.10% -11.21%
===========================================
Files 1957 1957
Lines 75594 75610 +16
Branches 8224 8224
===========================================
- Hits 51637 43177 -8460
- Misses 21849 30325 +8476
Partials 2108 2108
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.29% <62.96%> (+<0.01%)` | :arrow_up: |
| python | `59.64% <100.00%> (-23.18%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.44% <100.00%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/base.py](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `76.86% <100.00%> (-13.94%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.16% <100.00%> (-20.67%)` | :arrow_down: |
| [superset/db\_engine\_specs/trino.py](https://app.codecov.io/gh/apache/superset/pull/24241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RyaW5vLnB5) | `79.36% <100.00%> (-4.38%)` | :arrow_down: |
... and [303 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24241/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209397283
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24241:
URL: https://github.com/apache/superset/pull/24241#issuecomment-1568793786
@villebro for Trino the `Decimal` type is no longer experimental per https://github.com/trinodb/trino-python-client/pull/308, i.e., now by default these types use the native Python type.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209699364
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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 text 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.
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24241: fix(trino,mysql): handle string typed decimal results
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24241:
URL: https://github.com/apache/superset/pull/24241#discussion_r1209429016
##########
superset/db_engine_specs/base.py:
##########
@@ -737,7 +741,27 @@ def fetch_data(
try:
if cls.limit_method == LimitMethod.FETCH_MANY and limit:
return cursor.fetchmany(limit)
- return cursor.fetchall()
+ data = cursor.fetchall()
+ description = cursor.description or []
+ column_type_mutators = {
+ row[0]: func
+ for row in description
+ if (
+ func := cls.column_type_mutators.get(
+ type(cls.get_sqla_column_type(cls.get_datatype(row[1])))
+ )
+ )
+ }
+ if column_type_mutators:
Review Comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org