Repository navigation
fix(dynamodb): map NUMBER columns and order charts by metric expressions - #44711
Conversation
PyDynamoDB describes DynamoDB numbers as NUMBER, which no column type mapping matched, so numeric columns had no generic type. Its dialect also omits top-level column aliases (PartiQL has none), so an ORDER BY on a metric label (e.g. ORDER BY total) referenced a column that did not exist and sorted charts failed. Map NUMBER to Numeric and order by the expression (allows_alias_in_orderby = False).
Code Review Agent Run #449f26Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44711 +/- ##
==========================================
- Coverage 81.12% 81.12% -0.01%
==========================================
Files 2956 2956
Lines 178436 178440 +4
Branches 41341 41341
==========================================
+ Hits 144764 144766 +2
- Misses 30967 30969 +2
Partials 2705 2705
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM. Both changes are narrowly targeted and use real, existing hooks correctly, allows_alias_in_orderby is an established flag and column_type_mappings is empty on the base class so nothing's lost by not extending it. Good test coverage on the type mapping. Approving.
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Late to this one since it's merged, but I read it through and both fixes hold up. Dropped SELECT aliases are fine too, because query() renames the df columns positionally from labels_expected.
Left two follow-up notes inline: the series-limit join still depends on inner aliases, and the ORDER BY test only checks the flag.
| # 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 |
There was a problem hiding this 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.
| DynamoDBEngineSpec as spec, # noqa: N813 | ||
| ) | ||
|
|
||
| assert spec.allows_alias_in_orderby is False |
There was a problem hiding this 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.
3 participants
SUMMARY
Fixes two defects in
DynamoDBEngineSpec. Both block charts on a DynamoDB virtual dataset.NUMBER(pydynamodb.sql.common.DataTypes.NUMBER). No default column type mapping matches it, soget_column_spec("NUMBER")returnsNone. A numeric column in a dataset therefore gets notype_generic. This now maps toNumeric/GenericDataType.NUMERIC.SELECT label, SUM(amount), COUNT(*) FROM (...) AS virtual_table GROUP BY label ORDER BY total DESC: theAS totallabel is dropped, but theORDER BYstill references it. This fails withno such column: total(the superset connector evaluates it in SQLite). Withallows_alias_in_orderby = False, Superset orders by the expression instead:ORDER BY SUM(amount) DESC.Related: passren/PyDynamoDB#86 types
cursor.descriptionfrom the returned values. With it, a virtual dataset overSELECT id, amount, ts, labelgets NUMBER and DATETIME columns. With the mapping in this PR, those columns become NUMERIC and TEMPORAL.TESTING INSTRUCTIONS
pytest tests/unit_tests/db_engine_specs/test_dynamodb.py: 10 passed. On master, 3 of the new tests fail.connector=superset). The type fix from Type cursor.description columns from the returned values passren/PyDynamoDB#86 was applied, and the requests went through the REST API. The flow was: SQL LabSELECT, then a virtual dataset (id/amount NUMERIC, ts TEMPORAL withis_dttm), then a saved table chart withSUM(amount)andCOUNT(*)by label, ordered by the SUM metric, then a P1D time-grain query.no such column: total.ADDITIONAL INFORMATION