fitzee commented on code in PR #45389:
URL: https://github.com/apache/superset/pull/45389#discussion_r4321961052
##########
superset/semantic_layers/mapper.py:
##########
@@ -167,7 +168,9 @@ def _dispatch_semantic_query(
# ``SemanticResult(results=None)`` for zero rows, and a cached ``None``
# table would break every later transformation of that entry.
def normalized_dispatcher(dispatched_query: SemanticQuery) ->
SemanticResult:
- result: SemanticResult = execute_semantic_query(dispatcher,
dispatched_query)
+ result: SemanticResult = execute_semantic_query(
+ dispatcher, with_default_order(dispatched_query)
Review Comment:
**Non-blocking:** this also adds the default to `get_row_count` dispatches
(`dispatcher` is `semantic_view.get_row_count` when
`query_object.is_rowcount`). Order can't change a row count, even with a limit,
because the count of the first N rows is `min(N, total)` either way. So every
server-paginated table count now carries a sort it doesn't need. A provider
that builds `SELECT COUNT(*) FROM (<query>)` also ends up with an ORDER BY in a
derived table, which T-SQL-family engines reject unless there's TOP/OFFSET. I'd
apply the default only for `get_table`, and narrow the SDK sentence "A provider
therefore receives an empty `order` only when no temporal dimension is
selected" to match.
##########
superset/semantic_layers/cache_repository.py:
##########
@@ -344,6 +353,7 @@ def _is_registered(
)
return any(
entry.value_key == value_key
+ and entry.order_key == order_key
Review Comment:
**Non-blocking (test gap):** this check is what makes the "recomputed once
on their next request" upgrade claim true, and no test covers it. With this
line reverted, all 1751 semantic_layers tests still pass. Without it, an
ordinary miss stores with `replace=False`. That call would see the pre-upgrade
descriptor (same `value_key`, `order_key=""`) as registered and skip the store.
`select_reuse` then rejects that descriptor for EXACT/PROJECT, so every request
for that chart would go to the provider until the old entry's TTL expired.
Suggested test: register a descriptor with `order_key=""` for an unordered
temporal query, call `store(..., replace=False)`, and assert that the
descriptor is replaced and the next `lookup` returns an EXACT hit.
##########
superset/semantic_layers/cache_transform.py:
##########
@@ -197,6 +199,14 @@ def _rollup(frame: pd.DataFrame, query: SemanticQuery) ->
pd.DataFrame:
)
+def _sort_by_default_order(frame: pd.DataFrame, query: SemanticQuery) ->
pd.DataFrame:
+ """Restore the default order a rollup's grouping replaced, before the
limit."""
+ dimension: Dimension | None = default_order_dimension(query)
+ if dimension is None:
+ return frame
+ return frame.sort_values(dimension.name, kind="stable", ignore_index=True)
Review Comment:
**Nit:** `sort_values` puts NULL time values last. BigQuery, MySQL and
Spark/Databricks sort NULLs first for ASC. On a provider over one of those
engines, when the limit cuts, a rolled-up hit and a provider miss return
different rows for the same query. The SDK text leaves NULL placement
unspecified, so this is allowed, but it cuts against "cache hits follow the
same order". It's worth either a line in the docstring saying cache rollups
place NULLs last, or skipping limited rollups when the default column contains
nulls.
--
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]
