fix: Address regression introduced in #22853 #24121

john-bodley · 2023-05-18T19:13:02Z

SUMMARY

Per #22853 (comment), this PR addresses an issue introduced in #22853 where chart exploration from SQL Lab would instantiate TableColumn objects however there was no SqlaTable associated with said columns, which previously was a necessary requirement.

This PR augments the TableColumn class to include a Database object which is external to the ORM.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI and added some basic unit/integration tests.

Note there is very little unit/integration tests for this work beyond broad workflows. If people are accepting of this approach, I'll strive to add tests before merging, but I first thought there was merit to presenting this approach as a solution.

ADDITIONAL INFORMATION

Has associated issue:
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

john-bodley · 2023-05-18T22:02:27Z

superset/connectors/sqla/models.py

@@ -434,7 +463,7 @@ def get_sqla_col(
            expression = template_processor.process_template(expression)

        sqla_col: ColumnClause = literal_column(expression)
-        return self.table.make_sqla_column_compatible(sqla_col, label)
+        return self.table.database.make_sqla_column_compatible(sqla_col, label)


The SqlMetric class is still tightly bound to the SqlaTable.

codecov · 2023-05-18T22:04:08Z

Codecov Report

Merging #24121 (63ba034) into master (a3aacf2) will decrease coverage by 0.04%.
The diff coverage is 96.15%.

❗ Current head 63ba034 differs from pull request most recent head 60cb66a. Consider uploading reports for the commit 60cb66a to get more accurate results

@@            Coverage Diff             @@
##           master   #24121      +/-   ##
==========================================
- Coverage   69.05%   69.01%   -0.04%     
==========================================
  Files        1903     1903              
  Lines       74530    74522       -8     
  Branches     8105     8105              
==========================================
- Hits        51464    51434      -30     
- Misses      20955    20977      +22     
  Partials     2111     2111

Flag	Coverage Δ
hive	`53.83% <84.61%> (+0.03%)`	⬆️
mysql	`79.37% <96.15%> (+0.04%)`	⬆️
postgres	`79.44% <96.15%> (+0.04%)`	⬆️
presto	`?`
python	`83.26% <96.15%> (-0.07%)`	⬇️
sqlite	`77.96% <96.15%> (+0.04%)`	⬆️
unit	`54.25% <84.61%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
superset/models/sql_lab.py	`77.91% <0.00%> (-0.66%)`	⬇️
superset/connectors/sqla/models.py	`89.72% <100.00%> (+0.09%)`	⬆️
superset/initialization/__init__.py	`91.38% <100.00%> (+0.02%)`	⬆️
superset/models/core.py	`89.89% <100.00%> (-0.07%)`	⬇️

... and 2 files with indirect coverage changes

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

john-bodley · 2023-05-18T22:05:17Z

superset/connectors/sqla/models.py

@@ -1023,23 +1052,6 @@ def adhoc_column_to_sqla(  # pylint: disable=too-many-locals
            )
        return self.make_sqla_column_compatible(sqla_column, label)

-    def make_sqla_column_compatible(


The SqlaTable.make_sqla_column_compatible method was moved to Database.make_sqla_column_compatible given that now the TableColumn.table variable can be None.

john-bodley · 2023-05-18T22:38:12Z

superset/models/sql_lab.py

+        return [
+            TableColumn(
+                column_name=col["name"],
+                database=self.database,


This is a rewrite of the previous logic using a list comprehension. The only addition is the database argument which is required given a SQL Lab Query object has no associated SqlaTable which is previously were/how the database was obtained.

john-bodley · 2023-05-18T23:17:34Z

superset/connectors/sqla/models.py


    @property
    def type_generic(self) -> Optional[utils.GenericDataType]:
        if self.is_dttm:
            return GenericDataType.TEMPORAL

-        bool_types = ("BOOL",)


A nice byproduct of this change is this previous logic can be removed given that the db_engine_spec is accessible if associated with a Query or SqlaTable.

john-bodley · 2023-05-19T02:27:02Z

superset/connectors/sqla/models.py

@@ -229,6 +230,30 @@ class TableColumn(Model, BaseColumn, CertificationMixin):
    update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
    export_parent = "table"

+    def __init__(self, *args: Any, **kwargs: Any) -> None:


See https://docs.sqlalchemy.org/en/13/orm/constructors.html. Note I'm not overly sure if super is necessary—it's not included in the example, but I gather it doesn't hurt.

john-bodley · 2023-05-19T02:27:28Z

superset/connectors/sqla/models.py

+        of the ORM) depending on the context.
+        """
+
+        self._database = kwargs.pop("database", None)


Given that the database variable isn't part of the ORM I believe I'm correct in saying that it can only be defined via a keyword argument.

@john-bodley is there a follow up PR where'd you be using the database arg? also can you annotate this somehow to let people know this is going to be an ORM object vs some might assume it's an id

@hughhhh it's used in this PR here.

I've added a type annotation for clarity.

hughhhh · 2023-05-30T17:04:15Z

superset/connectors/sqla/models.py

+        is no longer true, i.e., the SqlaTable relationship is optional.
+
+        Now the TableColumn is either directly associated with the Database object (
+        which is unkwnonw to the ORM) or indirectly via the SqlaTable object (courtesy


john-bodley · 2023-05-30T23:38:43Z

superset/connectors/sqla/models.py

@@ -262,51 +287,33 @@ def is_temporal(self) -> bool:
            return self.is_dttm
        return self.type_generic == GenericDataType.TEMPORAL

+    @property
+    def database(self) -> Database:
+        return self.table.database if self.table else self._database


By design either self.table or self._database (but not both) will be defined and thus the database property will always be non-null.

john-bodley · 2023-05-31T03:08:14Z

@hughhhh I've addressed your comments.

pull-request-size bot added the size/M label May 18, 2023

john-bodley requested review from hughhhh, villebro and michael-s-molina May 18, 2023 19:13

john-bodley force-pushed the john-bodley--fix-22853 branch 2 times, most recently from 8116d38 to 4e6930c Compare May 18, 2023 21:37

john-bodley commented May 18, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-22853 branch from 4e6930c to 2f378e8 Compare May 18, 2023 23:16

pull-request-size bot added size/L and removed size/M labels May 18, 2023

john-bodley commented May 18, 2023

View reviewed changes

john-bodley requested a review from betodealmeida May 18, 2023 23:18

john-bodley force-pushed the john-bodley--fix-22853 branch from 2f378e8 to f184fdc Compare May 19, 2023 02:15

john-bodley commented May 19, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-22853 branch from f184fdc to dcfa292 Compare May 19, 2023 02:34

john-bodley requested a review from eschutho May 22, 2023 14:48

hughhhh reviewed May 30, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-22853 branch 2 times, most recently from 925ddae to 9c1ce2a Compare May 30, 2023 23:30

john-bodley commented May 30, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-22853 branch 3 times, most recently from 4251023 to 2ccf708 Compare May 31, 2023 02:10

fix: Address regression introduced in apache#22853

e10b514

john-bodley force-pushed the john-bodley--fix-22853 branch from 2ccf708 to e10b514 Compare May 31, 2023 03:08

hughhhh self-requested a review June 9, 2023 17:41

hughhhh approved these changes Jun 9, 2023

View reviewed changes

john-bodley added 2 commits June 12, 2023 11:19

Merge branch 'master' into john-bodley--fix-22853

46ee00e

Fix lint

60cb66a

john-bodley merged commit 2b36489 into apache:master Jun 12, 2023
29 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Address regression introduced in #22853 #24121

fix: Address regression introduced in #22853 #24121

john-bodley commented May 18, 2023 •

edited

john-bodley May 18, 2023

codecov bot commented May 18, 2023 •

edited

john-bodley May 18, 2023

john-bodley May 18, 2023

john-bodley May 18, 2023

john-bodley May 19, 2023

john-bodley May 19, 2023

hughhhh May 30, 2023

john-bodley May 30, 2023

hughhhh May 30, 2023

john-bodley May 30, 2023

john-bodley commented May 31, 2023

fix: Address regression introduced in #22853 #24121

fix: Address regression introduced in #22853 #24121

Conversation

john-bodley commented May 18, 2023 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

codecov bot commented May 18, 2023 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented May 31, 2023

john-bodley commented May 18, 2023 •

edited

codecov bot commented May 18, 2023 •

edited