fix(db_engine_specs): improve Presto column type matching #10658

villebro · 2020-08-23T07:55:54Z

SUMMARY

Currently some column types aren't correctly identified on Presto, as the matching pattern doesn't allow for field length (for varchar it fails to correctly identify varchar(255)). Since similar but more robust regex matching is already implemented on MSSQL, this aligns that handling on Presto and removes duplicated code. As of this PR, unidentified column types on Presto will now raise an exception to avoid these types of bugs going unnoticed, as the type coverage seems very comprehensive and is easy to expand as needed.

TEST PLAN

CI + new tests

ADDITIONAL INFORMATION

villebro · 2020-08-23T12:15:41Z

tests/db_engine_specs/presto_tests.py

+        sqla_type = PrestoEngineSpec.get_sqla_column_type("varchar(255)")
+        assert isinstance(sqla_type, types.VARCHAR)
+        assert sqla_type.length == 255
+
+        sqla_type = PrestoEngineSpec.get_sqla_column_type("varchar")
+        assert isinstance(sqla_type, types.String)


Here we can see that varchar(255) creates a 255 long VARCHAR object, whereas just varchar leaves the length undefined.

villebro · 2020-08-23T18:11:31Z

superset/db_engine_specs/presto.py

+        (
+            re.compile(r"^varchar(\((\d+)\))*$", re.IGNORECASE),
+            lambda match: types.VARCHAR(int(match[2])) if match[2] else types.String(),
+        ),


Here we're instantiating a types.VARCHAR object with a set length based on the regex match. This is usually unnecessary, but I thought I'd throw it in here to demonstrate how the matches from the regex can be used to customize the SQLA object.

villebro · 2020-08-23T18:13:52Z

superset/db_engine_specs/base.py

+    _column_type_mappings: Tuple[
+        Tuple[Pattern[str], Union[TypeEngine, Callable[[Match[str]], TypeEngine]]], ...,
+    ] = ()


Here the type can either be a SQLA type instance, or a callback that receives the match object and in turn returns the SQLA type instance.

dpgaspar

LGTM, just a comment

dpgaspar · 2020-08-24T09:06:09Z

superset/db_engine_specs/base.py

@@ -885,12 +890,18 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
        """
        Return a sqlalchemy native column type that corresponds to the column type
        defined in the data source (return None to use default type inferred by
-        SQLAlchemy). Needs to be overridden if column requires special handling
+        SQLAlchemy). Override `_column_type_mappings` for specific needs


If we expect subclasses to override _column_type_mappings should we make it public?

Agreed, the thought crossed my mind while writing this but there were other similar properties that were private so I went with the flow. I changed this one to public, and I'll update the other ones in a later PR to keep this PR as light as possible.

codecov-commenter · 2020-08-24T15:48:13Z

Codecov Report

Merging #10658 into master will decrease coverage by 4.31%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master   #10658      +/-   ##
==========================================
- Coverage   64.33%   60.01%   -4.32%     
==========================================
  Files         784      784              
  Lines       36952    36952              
  Branches     3529     3529              
==========================================
- Hits        23772    22178    -1594     
- Misses      13071    14587    +1516     
- Partials      109      187      +78

Flag	Coverage Δ
#cypress	`?`
#javascript	`60.83% <ø> (ø)`
#python	`59.53% <91.66%> (-0.26%)`	⬇️

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

Impacted Files	Coverage Δ
superset/models/sql_types/presto_sql_types.py	`79.48% <ø> (-1.01%)`	⬇️
superset/db_engine_specs/presto.py	`70.91% <86.66%> (-11.78%)`	⬇️
superset/db_engine_specs/base.py	`87.87% <100.00%> (+0.23%)`	⬆️
superset/db_engine_specs/mssql.py	`92.30% <100.00%> (-1.03%)`	⬇️
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/index.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/index.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/setup/setupColors.js	`0.00% <0.00%> (-100.00%)`	⬇️
... and 151 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 0177c2f...ca9f843. Read the comment docs.

bkyryliuk

Nice improvement, thank you!

bkyryliuk · 2020-08-24T18:02:17Z

tests/db_engine_specs/presto_tests.py

+
+    @staticmethod
+    def test_get_sqla_column_type():
+        sqla_type = PrestoEngineSpec.get_sqla_column_type("varchar(255)")


it would be nice to add some more test cases here especially the ones that are not supported by presto

Since this is blocking another fairly critical feature I'll defer the more comprehensive tests to a forthcoming PR and just cover the discovered bug here. I'm hoping I can carve out some time to build a template for testing this kind of functionality, and don't mind doing the initial work on the Presto spec.

bkyryliuk · 2020-08-24T18:02:49Z

tests/db_engine_specs/presto_tests.py

@@ -490,3 +491,24 @@ def test_presto_expand_data_array(self):
        self.assertEqual(actual_cols, expected_cols)
        self.assertEqual(actual_data, expected_data)
        self.assertEqual(actual_expanded_cols, expected_expanded_cols)
+
+    @staticmethod
+    def test_get_sqla_column_type():


no need to make it a class method, in pytest you can just do

def test_get_sqla_column_type(): assert True

Oh, not sure what I was doing there! I'll just tack it on as a regular class method like the other tests to limit confusion. Do we already have a test module that follows pytest best practices that I can mimic going forward?

yep @villebro - check
https://github.com/apache/incubator-superset/blob/master/tests/csv_upload_tests.py out

…ache#10658)" This reverts commit 9461f9c.

michellethomas · 2020-08-28T21:53:37Z

superset/db_engine_specs/base.py

        (see MSSQL for example of NCHAR/NVARCHAR handling).

        :param type_: Column type returned by inspector
        :return: SqlAlchemy column type
        """
+        for regex, sqla_type in cls.column_type_mappings:
+            match = regex.match(type_)


@villebro we tested this during a deploy and got errors here because sometimes type_ is None. get_sqla_column_type is used in connectors/sqla/models.py where sometimes the type on a column can be null.

…ache#10658)" This reverts commit 9461f9c.

* fix: improve Presto column type matching * add optional callback to type map and add tests * lint * change private to public

superset-github-bot bot added the preset-io label Aug 23, 2020

pull-request-size bot added the size/L label Aug 23, 2020

tooptoop4 mentioned this pull request Aug 23, 2020

fix: Get actual data type of presto columns (that have precision) in sql lab schema listing #9896

Closed

villebro changed the title ~~[WIP] fix: improve Presto column type matching~~ fix(db_engine_specs): improve Presto column type matching Aug 23, 2020

villebro commented Aug 23, 2020

View reviewed changes

villebro force-pushed the villebro/fix-presto-types branch from 4475922 to 495fc72 Compare August 23, 2020 13:53

villebro mentioned this pull request Aug 23, 2020

feat: enable metadata sync for virtual tables #10645

Merged

6 tasks

villebro force-pushed the villebro/fix-presto-types branch from 495fc72 to 5667d68 Compare August 23, 2020 17:54

villebro commented Aug 23, 2020

View reviewed changes

villebro requested review from bkyryliuk and john-bodley August 23, 2020 18:33

dpgaspar approved these changes Aug 24, 2020

View reviewed changes

bkyryliuk approved these changes Aug 24, 2020

View reviewed changes

villebro added 4 commits August 24, 2020 21:37

fix: improve Presto column type matching

bb66be6

add optional callback to type map and add tests

63308f4

lint

d0c043e

change private to public

ca9f843

villebro force-pushed the villebro/fix-presto-types branch from 23f3500 to ca9f843 Compare August 24, 2020 18:37

villebro merged commit 9461f9c into apache:master Aug 24, 2020

villebro deleted the villebro/fix-presto-types branch August 24, 2020 19:42

michellethomas pushed a commit to airbnb/superset-fork that referenced this pull request Aug 27, 2020

Revert "fix(db_engine_specs): improve Presto column type matching (ap…

76f010a

…ache#10658)" This reverts commit 9461f9c.

michellethomas reviewed Aug 28, 2020

View reviewed changes

michellethomas mentioned this pull request Aug 31, 2020

Error loading charts for presto table with null column types #10743

Closed

3 tasks

villebro mentioned this pull request Sep 1, 2020

fix(presto): default unknown types to string type #10753

Merged

6 tasks

serenajiang added a commit to airbnb/superset-fork that referenced this pull request Sep 2, 2020

Revert "fix(db_engine_specs): improve Presto column type matching (ap…

c3dd451

…ache#10658)" This reverts commit 9461f9c.

etr2460 pushed a commit to airbnb/superset-fork that referenced this pull request Sep 11, 2020

Revert "fix(db_engine_specs): improve Presto column type matching (ap…

2e0ae56

…ache#10658)" This reverts commit 9461f9c.

ktmud added a commit to ktmud/superset that referenced this pull request Sep 15, 2020

Revert "fix(db_engine_specs): improve Presto column type matching (ap…

f5b0844

…ache#10658)" This reverts commit 9461f9c.

ktmud added a commit to airbnb/superset-fork that referenced this pull request Sep 16, 2020

Revert "fix(db_engine_specs): improve Presto column type matching (ap…

869496d

…ache#10658)" This reverts commit 9461f9c.

dpgaspar added the v0.38 label Sep 22, 2020

dpgaspar pushed a commit that referenced this pull request Sep 22, 2020

fix(db_engine_specs): improve Presto column type matching (#10658)

4f3fcfd

* fix: improve Presto column type matching * add optional callback to type map and add tests * lint * change private to public

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(db_engine_specs): improve Presto column type matching #10658

fix(db_engine_specs): improve Presto column type matching #10658

villebro commented Aug 23, 2020 •

edited

Loading

villebro Aug 23, 2020

villebro Aug 23, 2020 •

edited

Loading

villebro Aug 23, 2020

dpgaspar left a comment

dpgaspar Aug 24, 2020

villebro Aug 24, 2020

codecov-commenter commented Aug 24, 2020 •

edited

Loading

bkyryliuk left a comment

bkyryliuk Aug 24, 2020

villebro Aug 24, 2020

bkyryliuk Aug 24, 2020

villebro Aug 24, 2020

bkyryliuk Aug 25, 2020

michellethomas Aug 28, 2020

fix(db_engine_specs): improve Presto column type matching #10658

fix(db_engine_specs): improve Presto column type matching #10658

Conversation

villebro commented Aug 23, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

villebro Aug 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 24, 2020 • edited Loading

Codecov Report

bkyryliuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro commented Aug 23, 2020 •

edited

Loading

villebro Aug 23, 2020 •

edited

Loading

codecov-commenter commented Aug 24, 2020 •

edited

Loading