mikebridge commented on code in PR #45389:
URL: https://github.com/apache/superset/pull/45389#discussion_r4327100377
##########
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:
Agreed. Row-count dispatches now go to the provider without the default
order, and they also skip the cache explicitly, since cache descriptors assume
the default was applied (1a931bb, 0244f5c). The SDK text now says the default
applies to `get_table` and that `get_row_count` gets none. An explicit chart
sort still reaches `get_row_count` with server pagination. That predates this
PR, and it is tracked as a follow-up.
##########
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:
Thanks, added
`test_store_replaces_a_descriptor_written_without_the_default_order` in
1a931bb. It stores a limited temporal query, rewrites the descriptor to
`order_key=""`, checks that lookup misses, then calls `store(...,
replace=False)` and asserts an EXACT hit. It fails with the `order_key`
comparison removed.
##########
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:
Went with the docstring: `_sort_by_default_order` now says the default
column sorts NULLs last, and that on a NULLs-first engine a limited rollup and
a provider miss can keep different rows (1a931bb). Making NULL placement
explicit is tracked as a follow-up.
--
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]
