superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ville...@apache.org
Subject [incubator-superset] branch master updated: [Bugfix] Remove prequery properties from query_obj (#7896)
Date Tue, 23 Jul 2019 19:14:10 GMT
This is an automated email from the ASF dual-hosted git repository.

villebro pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 07a76f8  [Bugfix] Remove prequery properties from query_obj (#7896)
07a76f8 is described below

commit 07a76f83b14df6c2fb343f7933dc87f53ecd5155
Author: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
AuthorDate: Tue Jul 23 22:13:58 2019 +0300

    [Bugfix] Remove prequery properties from query_obj (#7896)
    
    * Create query_obj for every filter
    
    * Deprecate is_prequery and prequeries from query_obj
    
    * Fix tests
    
    * Fix typos and remove redundant ; from sql
    
    * Add typing to namedtuples and move all query str logic to one place
    
    * Fix unit test
---
 superset/common/query_object.py     |  6 ----
 superset/connectors/druid/models.py |  2 --
 superset/connectors/sqla/models.py  | 61 +++++++++++++++++++------------------
 superset/db_engine_specs/base.py    |  6 ++--
 superset/db_engine_specs/druid.py   |  4 +--
 superset/db_engine_specs/gsheets.py |  4 +--
 superset/db_engine_specs/pinot.py   |  6 ++--
 superset/models/core.py             |  2 +-
 superset/views/core.py              |  7 +----
 superset/viz.py                     |  2 --
 tests/model_tests.py                | 13 +++-----
 tests/sqla_models_tests.py          |  1 -
 12 files changed, 48 insertions(+), 66 deletions(-)

diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index 7d72aa5..28f2303 100644
--- a/superset/common/query_object.py
+++ b/superset/common/query_object.py
@@ -48,8 +48,6 @@ class QueryObject:
         timeseries_limit_metric: Optional[Dict] = None,
         order_desc: bool = True,
         extras: Optional[Dict] = None,
-        prequeries: Optional[List[Dict]] = None,
-        is_prequery: bool = False,
         columns: List[str] = None,
         orderby: List[List] = None,
         relative_start: str = app.config.get("DEFAULT_RELATIVE_START_TIME", "today"),
@@ -78,8 +76,6 @@ class QueryObject:
         self.timeseries_limit = timeseries_limit
         self.timeseries_limit_metric = timeseries_limit_metric
         self.order_desc = order_desc
-        self.prequeries = prequeries if prequeries is not None else []
-        self.is_prequery = is_prequery
         self.extras = extras if extras is not None else {}
         self.columns = columns if columns is not None else []
         self.orderby = orderby if orderby is not None else []
@@ -97,8 +93,6 @@ class QueryObject:
             "timeseries_limit": self.timeseries_limit,
             "timeseries_limit_metric": self.timeseries_limit_metric,
             "order_desc": self.order_desc,
-            "prequeries": self.prequeries,
-            "is_prequery": self.is_prequery,
             "extras": self.extras,
             "columns": self.columns,
             "orderby": self.orderby,
diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py
index 6a0873c..d7b00c3 100644
--- a/superset/connectors/druid/models.py
+++ b/superset/connectors/druid/models.py
@@ -1120,8 +1120,6 @@ class DruidDatasource(Model, BaseDatasource):
         phase=2,
         client=None,
         order_desc=True,
-        prequeries=None,
-        is_prequery=False,
     ):
         """Runs a query against Druid and returns a dataframe.
         """
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index dfedca3..35b8590 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -15,10 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 # pylint: disable=C,R,W
-from collections import namedtuple, OrderedDict
+from collections import OrderedDict
 from datetime import datetime
 import logging
-from typing import Any, List, Optional, Union
+from typing import Any, List, NamedTuple, Optional, Union
 
 from flask import escape, Markup
 from flask_appbuilder import Model
@@ -45,7 +45,7 @@ from sqlalchemy.orm import backref, relationship
 from sqlalchemy.orm.exc import NoResultFound
 from sqlalchemy.schema import UniqueConstraint
 from sqlalchemy.sql import column, literal_column, table, text
-from sqlalchemy.sql.expression import Label, TextAsFrom
+from sqlalchemy.sql.expression import Label, Select, TextAsFrom
 import sqlparse
 
 from superset import app, db, security_manager
@@ -61,10 +61,18 @@ from superset.utils import core as utils, import_datasource
 config = app.config
 metadata = Model.metadata  # pylint: disable=no-member
 
-SqlaQuery = namedtuple(
-    "SqlaQuery", ["sqla_query", "labels_expected", "extra_cache_keys"]
-)
-QueryStringExtended = namedtuple("QueryStringExtended", ["sql", "labels_expected"])
+
+class SqlaQuery(NamedTuple):
+    extra_cache_keys: List[Any]
+    labels_expected: List[str]
+    prequeries: List[str]
+    sqla_query: Select
+
+
+class QueryStringExtended(NamedTuple):
+    labels_expected: List[str]
+    prequeries: List[str]
+    sql: str
 
 
 class AnnotationDatasource(BaseDatasource):
@@ -351,7 +359,7 @@ class SqlaTable(Model, BaseDatasource):
         """
         label_expected = label or sqla_col.name
         db_engine_spec = self.database.db_engine_spec
-        if db_engine_spec.supports_column_aliases:
+        if db_engine_spec.allows_column_aliases:
             label = db_engine_spec.make_label_compatible(label_expected)
             sqla_col = sqla_col.label(label)
         sqla_col._df_label_expected = label_expected
@@ -532,18 +540,20 @@ class SqlaTable(Model, BaseDatasource):
     def get_template_processor(self, **kwargs):
         return get_template_processor(table=self, database=self.database, **kwargs)
 
-    def get_query_str_extended(self, query_obj):
+    def get_query_str_extended(self, query_obj) -> QueryStringExtended:
         sqlaq = self.get_sqla_query(**query_obj)
         sql = self.database.compile_sqla_query(sqlaq.sqla_query)
         logging.info(sql)
         sql = sqlparse.format(sql, reindent=True)
-        if query_obj["is_prequery"]:
-            query_obj["prequeries"].append(sql)
         sql = self.mutate_query_from_config(sql)
-        return QueryStringExtended(labels_expected=sqlaq.labels_expected, sql=sql)
+        return QueryStringExtended(
+            labels_expected=sqlaq.labels_expected, sql=sql, prequeries=sqlaq.prequeries
+        )
 
     def get_query_str(self, query_obj):
-        return self.get_query_str_extended(query_obj).sql
+        query_str_ext = self.get_query_str_extended(query_obj)
+        all_queries = query_str_ext.prequeries + [query_str_ext.sql]
+        return ";\n\n".join(all_queries) + ";"
 
     def get_sqla_table(self):
         tbl = table(self.table_name)
@@ -606,8 +616,6 @@ class SqlaTable(Model, BaseDatasource):
         extras=None,
         columns=None,
         order_desc=True,
-        prequeries=None,
-        is_prequery=False,
     ):
         """Querying any sqla table from this common interface"""
         template_kwargs = {
@@ -624,6 +632,7 @@ class SqlaTable(Model, BaseDatasource):
         template_kwargs["extra_cache_keys"] = extra_cache_keys
         template_processor = self.get_template_processor(**template_kwargs)
         db_engine_spec = self.database.db_engine_spec
+        prequeries: List[str] = []
 
         orderby = orderby or []
 
@@ -793,7 +802,7 @@ class SqlaTable(Model, BaseDatasource):
             qry = qry.limit(row_limit)
 
         if is_timeseries and timeseries_limit and groupby and not time_groupby_inline:
-            if self.database.db_engine_spec.inner_joins:
+            if self.database.db_engine_spec.allows_joins:
                 # some sql dialects require for order by expressions
                 # to also be in the select clause -- others, e.g. vertica,
                 # require a unique inner alias
@@ -844,10 +853,8 @@ class SqlaTable(Model, BaseDatasource):
                         )
                     ]
 
-                # run subquery to get top groups
-                subquery_obj = {
-                    "prequeries": prequeries,
-                    "is_prequery": True,
+                # run prequery to get top groups
+                prequery_obj = {
                     "is_timeseries": False,
                     "row_limit": timeseries_limit,
                     "groupby": groupby,
@@ -861,7 +868,8 @@ class SqlaTable(Model, BaseDatasource):
                     "columns": columns,
                     "order_desc": True,
                 }
-                result = self.query(subquery_obj)
+                result = self.query(prequery_obj)
+                prequeries.append(result.query)
                 dimensions = [
                     c
                     for c in result.df.columns
@@ -873,9 +881,10 @@ class SqlaTable(Model, BaseDatasource):
                 qry = qry.where(top_groups)
 
         return SqlaQuery(
-            sqla_query=qry.select_from(tbl),
-            labels_expected=labels_expected,
             extra_cache_keys=extra_cache_keys,
+            labels_expected=labels_expected,
+            sqla_query=qry.select_from(tbl),
+            prequeries=prequeries,
         )
 
     def _get_timeseries_orderby(self, timeseries_limit_metric, metrics_dict, cols):
@@ -929,12 +938,6 @@ class SqlaTable(Model, BaseDatasource):
             db_engine_spec = self.database.db_engine_spec
             error_message = db_engine_spec.extract_error_message(e)
 
-        # if this is a main query with prequeries, combine them together
-        if not query_obj["is_prequery"]:
-            query_obj["prequeries"].append(sql)
-            sql = ";\n\n".join(query_obj["prequeries"])
-        sql += ";"
-
         return QueryResult(
             status=status,
             df=df,
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index eced6bb..d4a550c 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -117,9 +117,9 @@ class BaseEngineSpec(object):
     time_groupby_inline = False
     limit_method = LimitMethod.FORCE_LIMIT
     time_secondary_columns = False
-    inner_joins = True
-    allows_subquery = True
-    supports_column_aliases = True
+    allows_joins = True
+    allows_subqueries = True
+    allows_column_aliases = True
     force_column_alias_quotes = False
     arraysize = 0
     max_column_name_length = 0
diff --git a/superset/db_engine_specs/druid.py b/superset/db_engine_specs/druid.py
index 46095ee..8cd0c4c 100644
--- a/superset/db_engine_specs/druid.py
+++ b/superset/db_engine_specs/druid.py
@@ -22,8 +22,8 @@ class DruidEngineSpec(BaseEngineSpec):
     """Engine spec for Druid.io"""
 
     engine = "druid"
-    inner_joins = False
-    allows_subquery = True
+    allows_joins = False
+    allows_subqueries = True
 
     time_grain_functions = {
         None: "{col}",
diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py
index ef80679..d7b3bc7 100644
--- a/superset/db_engine_specs/gsheets.py
+++ b/superset/db_engine_specs/gsheets.py
@@ -22,5 +22,5 @@ class GSheetsEngineSpec(SqliteEngineSpec):
     """Engine for Google spreadsheets"""
 
     engine = "gsheets"
-    inner_joins = False
-    allows_subquery = False
+    allows_joins = False
+    allows_subqueries = False
diff --git a/superset/db_engine_specs/pinot.py b/superset/db_engine_specs/pinot.py
index 1a01fa3..132cb48 100644
--- a/superset/db_engine_specs/pinot.py
+++ b/superset/db_engine_specs/pinot.py
@@ -24,9 +24,9 @@ from superset.db_engine_specs.base import BaseEngineSpec, TimestampExpression
 
 class PinotEngineSpec(BaseEngineSpec):
     engine = "pinot"
-    allows_subquery = False
-    inner_joins = False
-    supports_column_aliases = False
+    allows_subqueries = False
+    allows_joins = False
+    allows_column_aliases = False
 
     # Pinot does its own conversion below
     time_grain_functions: Dict[Optional[str], str] = {
diff --git a/superset/models/core.py b/superset/models/core.py
index cb80611..5a98d5e 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -771,7 +771,7 @@ class Database(Model, AuditMixinNullable, ImportMixin):
 
     @property
     def allows_subquery(self):
-        return self.db_engine_spec.allows_subquery
+        return self.db_engine_spec.allows_subqueries
 
     @property
     def data(self):
diff --git a/superset/views/core.py b/superset/views/core.py
index c1bb285..3c652c3 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -1008,12 +1008,7 @@ class Superset(BaseSupersetView):
             logging.exception(e)
             return json_error_response(e)
 
-        if query_obj and query_obj["prequeries"]:
-            query_obj["prequeries"].append(query)
-            query = ";\n\n".join(query_obj["prequeries"])
-        if query:
-            query += ";"
-        else:
+        if not query:
             query = "No query."
 
         return self.json_response(
diff --git a/superset/viz.py b/superset/viz.py
index 52075be..b804ec9 100644
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -320,8 +320,6 @@ class BaseViz(object):
             "extras": extras,
             "timeseries_limit_metric": timeseries_limit_metric,
             "order_desc": order_desc,
-            "prequeries": [],
-            "is_prequery": False,
         }
         return d
 
diff --git a/tests/model_tests.py b/tests/model_tests.py
index 7acd4b6..cd43a8c 100644
--- a/tests/model_tests.py
+++ b/tests/model_tests.py
@@ -195,11 +195,11 @@ class SqlaTableModelTestCase(SupersetTestCase):
         ds_col.expression = None
         ds_col.python_date_format = None
         spec = self.get_database_by_id(tbl.database_id).db_engine_spec
-        if not spec.inner_joins and inner_join:
+        if not spec.allows_joins and inner_join:
             # if the db does not support inner joins, we cannot force it so
             return None
-        old_inner_join = spec.inner_joins
-        spec.inner_joins = inner_join
+        old_inner_join = spec.allows_joins
+        spec.allows_joins = inner_join
         arbitrary_gby = "state || gender || '_test'"
         arbitrary_metric = dict(
             label="arbitrary", expressionType="SQL", sqlExpression="COUNT(1)"
@@ -209,12 +209,10 @@ class SqlaTableModelTestCase(SupersetTestCase):
             metrics=[arbitrary_metric],
             filter=[],
             is_timeseries=is_timeseries,
-            prequeries=[],
             columns=[],
             granularity="ds",
             from_dttm=None,
             to_dttm=None,
-            is_prequery=False,
             extras=dict(time_grain_sqla="P1Y"),
         )
         qr = tbl.query(query_obj)
@@ -226,7 +224,7 @@ class SqlaTableModelTestCase(SupersetTestCase):
             self.assertIn("JOIN", sql.upper())
         else:
             self.assertNotIn("JOIN", sql.upper())
-        spec.inner_joins = old_inner_join
+        spec.allows_joins = old_inner_join
         self.assertIsNotNone(qr.df)
         return qr.df
 
@@ -258,7 +256,6 @@ class SqlaTableModelTestCase(SupersetTestCase):
             granularity=None,
             from_dttm=None,
             to_dttm=None,
-            is_prequery=False,
             extras={},
         )
         sql = tbl.get_query_str(query_obj)
@@ -285,7 +282,6 @@ class SqlaTableModelTestCase(SupersetTestCase):
             granularity=None,
             from_dttm=None,
             to_dttm=None,
-            is_prequery=False,
             extras={},
         )
 
@@ -306,7 +302,6 @@ class SqlaTableModelTestCase(SupersetTestCase):
             granularity=None,
             from_dttm=None,
             to_dttm=None,
-            is_prequery=False,
             extras={},
         )
 
diff --git a/tests/sqla_models_tests.py b/tests/sqla_models_tests.py
index bbf0d22..8f62127 100644
--- a/tests/sqla_models_tests.py
+++ b/tests/sqla_models_tests.py
@@ -52,7 +52,6 @@ class DatabaseModelTestCase(SupersetTestCase):
             "metrics": [],
             "is_timeseries": False,
             "filter": [],
-            "is_prequery": False,
             "extras": {"where": "(user != '{{ cache_key_wrapper('user_2') }}')"},
         }
         extra_cache_keys = table.get_extra_cache_keys(query_obj)


Mime
View raw message