fix: create virtual table with exotic type #19714

villebro · 2022-04-14T08:46:10Z

SUMMARY

This is an alternative implementation of #19701, which reuses the same data type inference logic that's already being used when creating physical datasets. While I did this fix I started adding types for the column type objects, but that turned into a HUGE rabbit hole going well beyond the scope of this PR. Therefore the changes in this PR are kept to a minimum to fix the issue at hand, but once this is merged I'll open up a PR that introduces proper types to to these code paths.

TESTING INSTRUCTIONS

create a dataset on BigQuery that contains at least a String array type (Type: STRING, Mode: REPEATED)
Add a single row to the dataset (can be all nulls)
Go to SQL Lab and run the following query select * from schema.table
Hit "Explore" and see an error.

ADDITIONAL INFORMATION

Has associated issue: closes got an error when save query as a virtual dataset #19609
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

villebro · 2022-04-14T08:50:13Z

superset/connectors/sqla/utils.py

@@ -86,7 +83,7 @@ def get_physical_table_metadata(
            col.update(
                {
                    "type": "UNKNOWN",
-                    "generic_type": None,
+                    "type_generic": None,


this was a bycatch (see line 79/76 above with the correct key) when I worked on adding proper TypedDicts for these objects (opening up separate PR for that one, but I thought it worthwhile to fix this error right away).

codecov · 2022-04-14T10:05:04Z

Codecov Report

Merging #19714 (7c9fa19) into master (8865656) will decrease coverage by 12.66%.
The diff coverage is 51.85%.

❗ Current head 7c9fa19 differs from pull request most recent head 3c99eba. Consider uploading reports for the commit 3c99eba to get more accurate results

@@             Coverage Diff             @@
##           master   #19714       +/-   ##
===========================================
- Coverage   66.51%   53.84%   -12.67%     
===========================================
  Files        1684     1684               
  Lines       64560    64561        +1     
  Branches     6625     6630        +5     
===========================================
- Hits        42939    34762     -8177     
- Misses      19926    28101     +8175     
- Partials     1695     1698        +3

Flag	Coverage Δ
hive	`52.69% <100.00%> (+<0.01%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.54% <100.00%> (-0.01%)`	⬇️
python	`56.64% <100.00%> (-25.79%)`	⬇️
sqlite	`?`
unit	`47.75% <66.66%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/views/components/SubMenu.tsx	`85.96% <ø> (ø)`
superset/connectors/sqla/models.py	`76.67% <ø> (-12.78%)`	⬇️
...perset-frontend/src/views/components/MenuRight.tsx	`60.63% <31.57%> (-6.03%)`	⬇️
...rset-ui-core/src/connection/SupersetClientClass.ts	`100.00% <100.00%> (ø)`
superset-frontend/src/views/CRUD/utils.tsx	`64.80% <100.00%> (+0.28%)`	⬆️
superset/connectors/sqla/utils.py	`82.02% <100.00%> (-8.80%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/key_value/commands/upsert.py	`0.00% <0.00%> (-89.59%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-89.37%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-85.30%)`	⬇️
... and 273 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8865656...3c99eba. Read the comment docs.

michael-s-molina · 2022-04-14T10:54:33Z

superset/connectors/sqla/utils.py

@@ -231,7 +219,7 @@ def load_or_create_tables(  # pylint: disable=too-many-arguments
                    name=column["name"],
                    type=str(column["type"]),
                    expression=conditional_quote(column["name"]),
-                    is_temporal=is_column_type_temporal(column["type"]),
+                    is_temporal=column["is_dttm"],


The previous version checked for date, datetime, time and timedelta. Is this equivalent?

yes - this one is actually based on what the db engine spec says is temporal, so it's much more comprehensive (=can be customized per database).

michael-s-molina

LGTM. Thanks for the fix!

zhaoyongjie

Tested in my local with PG12. LGTM.

I found another issue when I was testing, SQLLab try to parse commented SQL.

* fix: create virtual table with exotic type * fix test (cherry picked from commit 2b17ce2)

sadpandajoe · 2022-04-18T22:35:21Z

🏷️ preset:2022.15

* fix: create virtual table with exotic type * fix test

fix: create virtual table with exotic type

918d35d

superset-github-bot bot added the preset-io label Apr 14, 2022

pull-request-size bot added the size/M label Apr 14, 2022

villebro mentioned this pull request Apr 14, 2022

fix: convert column type to str during dual read/write #19701

Closed

9 tasks

villebro commented Apr 14, 2022

View reviewed changes

fix test

3c99eba

villebro force-pushed the villebro/compile-nulltype branch from 1a0268d to 3c99eba Compare April 14, 2022 10:47

michael-s-molina reviewed Apr 14, 2022

View reviewed changes

michael-s-molina approved these changes Apr 14, 2022

View reviewed changes

zhaoyongjie self-requested a review April 14, 2022 11:57

zhaoyongjie approved these changes Apr 14, 2022

View reviewed changes

hughhhh self-requested a review April 14, 2022 14:52

hughhhh approved these changes Apr 14, 2022

View reviewed changes

villebro merged commit 2b17ce2 into apache:master Apr 14, 2022

villebro deleted the villebro/compile-nulltype branch April 14, 2022 14:52

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 14, 2022

fix: create virtual table with exotic type (apache#19714)

a3f1d4a

* fix: create virtual table with exotic type * fix test (cherry picked from commit 2b17ce2)

czgu mentioned this pull request Apr 16, 2022

column["is_dtttm"] is not found for Presto columns #19737

Closed

3 tasks

superset-github-bot bot added the preset:2022.15 label Apr 18, 2022

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

fix: create virtual table with exotic type (apache#19714)

63650bc

* fix: create virtual table with exotic type * fix test

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: create virtual table with exotic type #19714

fix: create virtual table with exotic type #19714

villebro commented Apr 14, 2022 •

edited

Loading

villebro Apr 14, 2022

codecov bot commented Apr 14, 2022 •

edited

Loading

michael-s-molina Apr 14, 2022

villebro Apr 14, 2022

michael-s-molina left a comment

zhaoyongjie left a comment

sadpandajoe commented Apr 18, 2022

fix: create virtual table with exotic type #19714

fix: create virtual table with exotic type #19714

Conversation

villebro commented Apr 14, 2022 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro Apr 14, 2022

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2022 • edited Loading

Codecov Report

michael-s-molina Apr 14, 2022

Choose a reason for hiding this comment

villebro Apr 14, 2022

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

sadpandajoe commented Apr 18, 2022

villebro commented Apr 14, 2022 •

edited

Loading

codecov bot commented Apr 14, 2022 •

edited

Loading