Repository navigation
feat(semantic-layer): sort NULL dates last in the default order - #45912
Open
mikebridge wants to merge 2 commits into
Open
mikebridge wants to merge 2 commits into
mikebridge wants to merge 2 commits into
Conversation
An unordered semantic query is sorted by its first temporal dimension ascending, but the SDK left NULL placement to the provider while the cache rollup put NULL values last. With a limit, a provider that sorts NULL first and a cached rollup could return different rows. The SDK order term can now carry an optional NULL placement: OrderTerm(member, direction, nulls) still unpacks as a (member, direction) pair, so existing providers keep working, and null_placement(term) reads it from either form. The host sends its generated default as ascending with NULL values last, which also tells it apart from an explicit ascending order on the same dimension. Explicit orders are unchanged. The cache identity includes the placement and moves to v6, so cached semantic results are recomputed once. The conformance kit adds a required null_placement case.
dataclasses.asdict and astuple rebuild a tuple from its elements, which failed for OrderTerm. The term now takes their named-tuple path, keeping its member and direction; its docstring says the placement is not carried and to read it with null_placement. The conformance kit names a missing query case instead of failing with a bare KeyError, and the provider testing guide asks the null_placement fixture for distinct key values and a single NULL group, since tied rows may come back in either order.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45912 +/- ##
==========================================
+ Coverage 84.73% 84.86% +0.13%
==========================================
Files 3069 3074 +5
Lines 205771 208832 +3061
Branches 48396 49070 +674
==========================================
+ Hits 174354 177229 +2875
- Misses 28123 28270 +147
- Partials 3294 3333 +39
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mikebridge
marked this pull request as ready for review
November 2, 2026 15:01
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| The page must keep different rows from the ones the opposite placement would. | ||
| """ | ||
| assert query.order, "null_placement fixture requires an order" | ||
| first: tuple[object, ...] = query.order[0] |
Contributor
There was a problem hiding this comment.
Suggestion: The required case only inspects query.order[0], so a provider that ignores NULL placement on later OrderTerms can pass despite the per-term SDK contract.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/semantic_layers/testing.py
**Line:** 303:303
**Comment:**
*Incomplete Implementation: The required case only inspects `query.order[0]`, so a provider that ignores NULL placement on later `OrderTerm`s can pass despite the per-term SDK contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
TL;DR: When a semantic query has no sort order, Superset sorts it by its first date column, oldest first. NULL dates now always go last, both from the provider and from Superset's cache, so a limited chart shows the same rows either way. Upgrading recomputes every cached semantic-layer result once, because the cache key format changes.
Background:
Problem:
[NULL, January, February]and a limit of 2, a NULL-first provider returns NULL and January, but a cache rollup returns January and February.Changes:
superset-core): an order term can now carry an optional NULL placement.OrderTerm(member, direction, nulls=None)still unpacks as a(member, direction)pair, so existing providers keep working unchanged.null_placement(term)reads the placement from either form;Nonemeans the provider's default, as before.OrderTerm(dimension, ASC, NullPlacement.LAST). Explicit orders are sent exactly as given and keep the provider's NULL placement.v6, so existing semantic cache entries are recomputed once.null_placementquery case checks that a provider applies a placement beforeoffset/limit. The SQLite reference provider rendersNULLS FIRST/LAST.UPDATING.mdcovers the provider obligation, the kit case and the one-time cache recompute;BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; no UI change. For a chart whose time column has NULL values and that has a row limit:
TESTING INSTRUCTIONS
pytest superset-core/tests/semantic_layers tests/unit_tests/semantic_layersdefault_order_test.py::test_default_order_differs_from_an_explicit_ascending_order;test_limited_hit_is_not_shared_between_default_and_explicit_ascending;test_conformance.py::...::test_query_outputs[null_placement]against a provider that ignores the placement.ADDITIONAL INFORMATION
OrderTerm,NullPlacement,null_placementin the semantic-layer SDK; a new required conformance-kit case)Notes for reviewers:
UPDATING.md. Both are textual.dataclasses.asdict: it works on a query holding anOrderTermbut keeps only member and direction (documented on the type).