villebro commented on a change in pull request #9824:
URL:
https://github.com/apache/incubator-superset/pull/9824#discussion_r426380594
##########
File path: superset/connectors/base/models.py
##########
@@ -182,7 +182,7 @@ def short_data(self) -> Dict[str, Any]:
}
@property
- def select_star(self):
+ def select_star(self) -> str:
pass
Review comment:
This probably needs to be `Optional[str]` or alternatively raise
`NotImplementedError`.
##########
File path: superset/typing.py
##########
@@ -16,15 +16,31 @@
# under the License.
from typing import Any, Callable, Dict, List, Optional, Tuple, Union
-from flask import Flask
+import flask
+import werkzeug
from flask_caching import Cache
-CacheConfig = Union[Callable[[Flask], Cache], Dict[str, Any]]
+CacheConfig = Union[Callable[[flask.Flask], Cache], Dict[str, Any]]
DbapiDescriptionRow = Tuple[
str, str, Optional[str], Optional[str], Optional[int], Optional[int], bool
]
DbapiDescription = Union[List[DbapiDescriptionRow], Tuple[DbapiDescriptionRow,
...]]
DbapiResult = List[Union[List[Any], Tuple[Any, ...]]]
FilterValue = Union[float, int, str]
FilterValues = Union[FilterValue, List[FilterValue], Tuple[FilterValue]]
+Granularity = Union[str, Dict[str, Union[str, float]]]
+Metric = Union[Dict[str, str], str]
+QueryObject = Dict[str, Any]
Review comment:
We should probably call this `QueryObjectDict` to distinguish from
`superset.common.query_object.QueryObject`.
##########
File path: superset/connectors/base/models.py
##########
@@ -93,7 +93,7 @@ class BaseDatasource(
update_from_object_fields: List[str]
@declared_attr
- def slices(self):
+ def slices(self) -> relationship:
Review comment:
I believe this should be `RelationshipProperty`. See:
https://docs.sqlalchemy.org/en/13/orm/relationship_api.html#sqlalchemy.orm.relationship
##########
File path: superset/connectors/druid/models.py
##########
@@ -558,8 +569,8 @@ def get_perm(self) -> str:
obj=self
)
- def update_from_object(self, obj):
- return NotImplementedError()
Review comment:
😄
##########
File path: superset/connectors/druid/models.py
##########
@@ -1322,13 +1347,13 @@ def run_query( # druid
set([x for x in pre_qry_dims if not isinstance(x, dict)])
)
dict_dims = [x for x in pre_qry_dims if isinstance(x, dict)]
- pre_qry["dimensions"] = non_dict_dims + dict_dims
+ pre_qry["dimensions"] = non_dict_dims + dict_dims # type:
ignore
- order_by = None
+ order_by = None # type: ignore
Review comment:
If we define this as `order_by: Optional[Metric]`, we can probably omit
the ignore?
##########
File path: superset/connectors/druid/models.py
##########
@@ -817,7 +829,7 @@ def granularity(
"year": "P1Y",
}
- granularity: Dict[str, Union[str, float]] = {"type": "period"}
+ granularity = {"type": "period"}
Review comment:
I would have thought this turns into a `Dict[str, str]` unless being
explicitly being defined as `Granularity`, and thus causing problems below when
numeric values are added?
##########
File path: superset/connectors/druid/models.py
##########
@@ -449,7 +460,7 @@ def get_perm(self) -> Optional[str]:
@classmethod
def import_obj(cls, i_metric: "DruidMetric") -> "DruidMetric":
- def lookup_obj(lookup_metric: DruidMetric) -> Optional[DruidMetric]:
+ def lookup_obj(lookup_metric: "DruidMetric") ->
Optional["DruidMetric"]:
Review comment:
Not sure how a nested function referencing its parent method's class is
handled by mypy, but if this was working before, we should probably be ok
leaving this without quotes?
##########
File path: superset/connectors/druid/models.py
##########
@@ -1240,7 +1268,7 @@ def run_query( # druid
qry["limit"] = row_limit
client.scan(**qry)
elif (IS_SIP_38 and columns) or (
- not IS_SIP_38 and len(groupby) == 0 and not having_filters
+ not IS_SIP_38 and (not groupby or len(groupby) == 0) and not
having_filters
Review comment:
I wonder if a simple falsy check here would be more pythonic: `not
groupby`? I can't think of an edge case for which it wouldn't give the correct
results.
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
