Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow adhoc columns in non-aggregate query #21729

Merged
merged 11 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@
from superset.superset_typing import (
AdhocColumn,
AdhocMetric,
Column as ColumnTyping,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mayurnewase out of interest why did you rename this? There doesn't seem to be a name clash per your change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-bodley there is import of Column from sqlalchemy on line 50.

Metric,
OrderBy,
QueryObjectDict,
Expand Down Expand Up @@ -1242,7 +1243,7 @@ def text(self, clause: str) -> TextClause:
def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-many-branches,too-many-statements
self,
apply_fetch_values_predicate: bool = False,
columns: Optional[List[Column]] = None,
columns: Optional[List[ColumnTyping]] = None,
extras: Optional[Dict[str, Any]] = None,
filter: Optional[ # pylint: disable=redefined-builtin
List[QueryObjectFilterClause]
Expand Down Expand Up @@ -1438,15 +1439,24 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
select_exprs.append(outer)
elif columns:
for selected in columns:
if is_adhoc_column(selected):
_sql = selected["sqlExpression"]
_column_label = selected["label"]
elif isinstance(selected, str):
_sql = selected
_column_label = selected

selected = validate_adhoc_subquery(
selected,
_sql,
self.database_id,
self.schema,
)
select_exprs.append(
columns_by_name[selected].get_sqla_col()
if selected in columns_by_name
else self.make_sqla_column_compatible(literal_column(selected))
if isinstance(selected, str) and selected in columns_by_name
else self.make_sqla_column_compatible(
literal_column(selected), _column_label
)
)
metrics_exprs = []

Expand Down
4 changes: 2 additions & 2 deletions superset/superset_typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ class AdhocMetric(TypedDict, total=False):

class AdhocColumn(TypedDict, total=False):
hasCustomLabel: Optional[bool]
label: Optional[str]
zhaoyongjie marked this conversation as resolved.
Show resolved Hide resolved
sqlExpression: Optional[str]
label: str
sqlExpression: str
columnType: Optional[Literal["BASE_AXIS", "SERIES"]]
timeGrain: Optional[str]

Expand Down
4 changes: 3 additions & 1 deletion superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -1271,7 +1271,9 @@ def is_adhoc_metric(metric: Metric) -> TypeGuard[AdhocMetric]:


def is_adhoc_column(column: Column) -> TypeGuard[AdhocColumn]:
return isinstance(column, dict)
return isinstance(column, dict) and ({"label", "sqlExpression"}).issubset(
column.keys()
)


def get_base_axis_labels(columns: Optional[List[Column]]) -> Tuple[str, ...]:
Expand Down
41 changes: 41 additions & 0 deletions tests/integration_tests/charts/data/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,47 @@ def test_with_virtual_table_with_colons_as_datasource(self):
assert "':xyz:qwerty'" in result["query"]
assert "':qwerty:'" in result["query"]

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_with_table_columns_without_metrics(self):
mayurnewase marked this conversation as resolved.
Show resolved Hide resolved
request_payload = self.query_context_payload
request_payload["queries"][0]["columns"] = ["name", "gender"]
request_payload["queries"][0]["metrics"] = None
request_payload["queries"][0]["orderby"] = []

rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
result = rv.json["result"][0]

assert rv.status_code == 200
assert "name" in result["colnames"]
assert "gender" in result["colnames"]
assert "name" in result["query"]
assert "gender" in result["query"]
assert list(result["data"][0].keys()) == ["name", "gender"]

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_with_adhoc_column_without_metrics(self):
request_payload = self.query_context_payload
request_payload["queries"][0]["columns"] = [
"name",
{
"label": "num divide by 10",
"sqlExpression": "num/10",
"expressionType": "SQL",
},
]
request_payload["queries"][0]["metrics"] = None
request_payload["queries"][0]["orderby"] = []

rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
result = rv.json["result"][0]

assert rv.status_code == 200
assert "num divide by 10" in result["colnames"]
assert "name" in result["colnames"]
assert "num divide by 10" in result["query"]
assert "name" in result["query"]
assert list(result["data"][0].keys()) == ["name", "num divide by 10"]


@pytest.mark.chart_data_flow
class TestGetChartDataApi(BaseTestChartDataApi):
Expand Down