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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
