EnxDev commented on code in PR #44711:
URL: https://github.com/apache/superset/pull/44711#discussion_r4137491661
##########
tests/unit_tests/db_engine_specs/test_dynamodb.py:
##########
@@ -42,3 +44,41 @@ def test_convert_dttm(
)
assert_convert_dttm(spec, target_type, expected_result, dttm)
+
+
[email protected](
+ "native_type,sqla_type,generic_type",
+ [
+ ("NUMBER", types.Numeric, GenericDataType.NUMERIC),
+ ("number", types.Numeric, GenericDataType.NUMERIC),
+ ("STRING", types.String, GenericDataType.STRING),
+ ("DATETIME", types.DateTime, GenericDataType.TEMPORAL),
+ ("DATE", types.Date, GenericDataType.TEMPORAL),
+ ("BOOL", types.Boolean, GenericDataType.BOOLEAN),
+ ],
+)
+def test_get_column_spec(
+ native_type: str,
+ sqla_type: type[types.TypeEngine],
+ generic_type: GenericDataType,
+) -> None:
+ from superset.db_engine_specs.dynamodb import (
+ DynamoDBEngineSpec as spec, # noqa: N813
+ )
+
+ column_spec = spec.get_column_spec(native_type)
+ assert column_spec is not None
+ assert isinstance(column_spec.sqla_type, sqla_type)
+ assert column_spec.generic_type == generic_type
+
+
+def test_orders_by_expression_not_alias() -> None:
+ """
+ The PyDynamoDB dialect drops top-level aliases, so ORDER BY must not
+ reference one.
+ """
+ from superset.db_engine_specs.dynamodb import (
+ DynamoDBEngineSpec as spec, # noqa: N813
+ )
+
+ assert spec.allows_alias_in_orderby is False
Review Comment:
Nit, take it or leave it. This asserts the flag we just set, so it can't
catch a regression in how the ORDER BY actually gets compiled.
A test that builds a query on a DynamoDB-backed table with a labeled metric
and checks the SQL has `ORDER BY SUM(amount) DESC` would guard the actual bug
from the description.
##########
superset/db_engine_specs/dynamodb.py:
##########
@@ -52,6 +54,20 @@ class DynamoDBEngineSpec(BaseEngineSpec):
"docs_url": "https://github.com/passren/PyDynamoDB";,
}
+ # The PyDynamoDB dialect omits top-level column aliases (PartiQL has none),
+ # so an ORDER BY on a SELECT alias (e.g. a metric label) names a column
that
+ # does not exist. Order by the expression instead.
+ allows_alias_in_orderby = False
Review Comment:
This fixes the main ORDER BY, but the series-limit subquery in `helpers.py`
(around line 5596) still selects `label__` / `mme_inner__` aliases and joins on
`label = label__`. The dialect strips those aliases too, so a timeseries chart
with a dimension and a series limit should still fail with `no such column`.
Might be misreading the connector, but would `allows_joins = False` help
here? It would route that case through the prequery path, which goes through
`query()` and gets the positional column rename.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
