Repository navigation
feat(semantic-layer): sort NULL dates last in the default order - #45912
mikebridge wants to merge 3 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.83% +0.10%
==========================================
Files 3069 3075 +6
Lines 205771 207867 +2096
Branches 48396 48739 +343
==========================================
+ Hits 174354 176341 +1987
- Misses 28123 28166 +43
- Partials 3294 3360 +66
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. |
![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] |
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 fixThere was a problem hiding this comment.
get_table requires "including any term's NULL placement" (view.py:175). Dropping placement only after the first key still passes all 65 conformance tests; add a tied-prefix multi-key witness whose page changes when secondary placement is dropped.
There was a problem hiding this comment.
Added a required secondary-order witness in 762801b. January rows tie on month; with category NULLS LAST, offset 1 / limit 1 keeps the NULL category (total 3). Dropping only the second key’s placement instead keeps A (total 2). The check partitions by the preceding keys, and rejects fixtures whose page cannot distinguish the two placements. There is also a descending/NULLS FIRST mutation check.
- Before: keeping placement only on the first key passed all 65 existing conformance tests.
- After: the same mutation fails the new required case while those 65 tests still pass.
- Verification: 1,985 PR tests passed, with 4 capability skips; changed-file hooks and the pre-push coverage gate passed.
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
The NULL-placement witness needs another adjustment.
| index: int | ||
| row: dict[str, object] | ||
| for index, row in enumerate(rows): | ||
| groups.setdefault(tuple(row[name] for name in prefix), []).append(index) |
There was a problem hiding this comment.
STRUCT/LIST prefix dimensions become dict/list values in to_pylist(), so this required case raises TypeError: unhashable type even when ordering is correct. Keep the hashable Arrow scalars as prefix keys.
| groups.setdefault(tuple(row[name] for name in prefix), []).append(index) | |
| groups.setdefault( | |
| tuple(full.column(name)[index] for name in prefix), [] | |
| ).append(index) |
| assert sorted(repr(rows[index]) for index in placed[start:stop]) != sorted( | ||
| repr(rows[index]) for index in opposite[start:stop] |
There was a problem hiding this comment.
The full-row check accepts (A,x),(NULL,y),(NULL,z) at offset 1/limit 1, although either placement may return (NULL,y) because NULL ties are unordered. The old key-value check rejected this non-discriminating fixture.
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).