[GitHub] [superset] villebro commented on a change in pull request #13434: fix(query): order by adhoc metrics should trigger group by

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #13434:
URL: https://github.com/apache/superset/pull/13434#discussion_r595472726



##########
File path: superset/connectors/sqla/models.py
##########
@@ -67,14 +68,16 @@
 from superset.models.helpers import AuditMixinNullable, QueryResult
 from superset.result_set import SupersetResultSet
 from superset.sql_parse import ParsedQuery
-from superset.typing import Metric, QueryObjectDict
+from superset.typing import AdhocMetric, Metric, OrderBy, QueryObjectDict
 from superset.utils import core as utils
 from superset.utils.core import GenericDataType
 
 config = app.config
 metadata = Model.metadata  # pylint: disable=no-member
 logger = logging.getLogger(__name__)
 
+VIRTUAL_TABLE_ALIAS = "expr_qry"

Review comment:
       I've always thought "expression query" sounds funny, as this is really just a "subquery". However, since we have other subqueries as well, maybe that's not the best name. "virtual_table" could be ok, as we use that term throughout the application.

##########
File path: superset/common/query_object.py
##########
@@ -166,11 +166,11 @@ def __init__(
         #   1. 'metric_name'   - name of predefined metric
         #   2. { label: 'label_name' }  - legacy format for a predefined metric
         #   3. { expressionType: 'SIMPLE' | 'SQL', ... } - adhoc metric
-        self.metrics = [
-            metric
-            if isinstance(metric, str) or "expressionType" in metric
-            else metric["label"]  # type: ignore
-            for metric in metrics
+        self.metrics = metrics and [
+            x

Review comment:
       nit: I'd personally prefer `metric_` here

##########
File path: superset/common/query_object.py
##########
@@ -236,7 +237,7 @@ def __init__(
     @property
     def metric_names(self) -> List[str]:
         """Return metrics names (labels), coerce adhoc metrics to strings."""
-        return get_metric_names(self.metrics)
+        return get_metric_names(self.metrics or [])

Review comment:
       I'm almost wondering if we should be returning `None` here if `self.metrics is None`

##########
File path: superset/connectors/sqla/models.py
##########
@@ -965,19 +983,47 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
             main_metric_expr, label = literal_column("COUNT(*)"), "ccount"
             main_metric_expr = self.make_sqla_column_compatible(main_metric_expr, label)
 
-        select_exprs: List[Column] = []
-        groupby_exprs_sans_timestamp = OrderedDict()
+        # To ensure correct handling of the ORDER BY labeling we need to reference the
+        # metric instance if defined in the SELECT clause.
+        metrics_exprs_by_label = {
+            m.name: m for m in metrics_exprs  # pylint: disable=protected-access
+        }
 
-        assert extras is not None
+        # Since orderby may use adhoc metrics, too; we need to process them first
+        orderby_exprs: List[ColumnElement] = []
+        for orig_col, ascending in orderby:
+            col: Optional[Union[Metric, ColumnElement]] = orig_col

Review comment:
       Can `col` really be `Optional` in this context?  I thought it's always defined.

##########
File path: superset/common/query_object.py
##########
@@ -116,24 +117,24 @@ def __init__(
         order_desc: bool = True,
         extras: Optional[Dict[str, Any]] = None,
         columns: Optional[List[str]] = None,
-        orderby: Optional[List[List[str]]] = None,
+        orderby: Optional[List[OrderBy]] = None,
         post_processing: Optional[List[Optional[Dict[str, Any]]]] = None,
         is_rowcount: bool = False,
         **kwargs: Any,
     ):
+        columns = columns or []
+        groupby = groupby or []
+        extras = extras or {}
+        annotation_layers = annotation_layers or []
+
         self.is_rowcount = is_rowcount
         self.datasource = None
         if datasource:
             self.datasource = ConnectorRegistry.get_datasource(
                 str(datasource["type"]), int(datasource["id"]), db.session
             )
         self.result_type = result_type
-        annotation_layers = annotation_layers or []
         self.apply_fetch_values_predicate = apply_fetch_values_predicate or False
-        metrics = metrics or []

Review comment:
       I think `[]` and `None` are used fairly interchangeably in the codebase, but I agree that setting `x = None` for cases where `x` is not relevant communicates intent better.




----------------------------------------------------------------
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.

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