mikebridge opened a new pull request, #45108:
URL: https://github.com/apache/superset/pull/45108
### SUMMARY
The semantic-layer SDK in `superset-core` publishes its value types but not
the rules that govern them. Member identity, grain-variant choice, NULL
semantics in filters and ordering-before-limit are all left implicit, so each
provider and several host modules re-derive them, and the same rule has
recently been fixed in more than one place at a time.
This writes each rule down once, in the docstring of the type that owns it.
No behavior changes.
- **`Metric` / `Dimension` — member identity.** `name` is the stable
selection key that saved charts and API payloads reference; `verbose_name` is
display only and excluded from equality; `id` is provider-private and no host
code reads it. The two kinds of uniqueness are now distinguished explicitly,
because conflating them is misleading: in the *catalog*, dimensions are unique
per `(name, grain)`, so several grain variants sharing one name are expected
and supported, while metric names are unique on their own and must not collide
with a dimension name; in a *query*, at most one variant per name is selected,
so a result carries one column per selected member name.
- **`Grains` — grain-variant selection.** When a selection does not name an
explicit supported grouping grain, one variant has to be chosen, and the
preference is: the raw variant, then the finest known grain (second through
year), then any remaining representation in lexical order. The docstring also
records why grains must not be sorted by `Grain.name` — alphabetically "Day"
precedes "Hour" and "Quarter" precedes "Week", the opposite of fineness.
Routing every host lookup through that single preference is the work of #44454;
until that lands, some call sites still choose their own way, so the text says
a provider should not rely on which variant a given lookup picks today.
- **`Operator` — NULL in filters.** `None` is not a valid comparison
operand: `= NULL`, `!= NULL`, `IN (NULL)` and `NOT IN (NULL)` are never true in
SQL, so a provider that renders or binds one silently drops every row the user
asked for, with no error. Nullness is expressed with `IS_NULL`/`IS_NOT_NULL`,
and an empty `IN` has no well-defined rendering either. Splitting a `None`
operand out into those predicates is the host's responsibility and is being
added separately, so the text tells a provider that already handles `None`
correctly to keep doing so until then — the docstring is deliberately not an
invitation to delete working provider-side handling.
- **`SemanticQuery` and `SemanticView.get_table` — ordering before limit.**
`order` applies to the full result set before `offset` and `limit`; truncating
first and sorting the page returns a different and wrong answer.
It also adds `superset.semantic_layers.members.members_by_key`, so member
identity has one owner in the host instead of being re-derived at each lookup,
and uses it at the mapper's three name-keyed lookups. The helper reproduces the
existing comprehension exactly — the mappings it returns are byte-identical,
including last-wins on duplicate names and insertion order — and a test pins
that equivalence. It deliberately takes an iterable of members rather than the
view, so `get_metrics()`/`get_dimensions()` are still called exactly once each
and the mapper can keep reusing the dimension set for the grain-variant loop
below.
Narrowing those lookups let MyPy see that `ValidatedQueryObject` declared
`metrics` wider than `_validate_metrics` permits, so the invariant that
validation already enforces is now recorded on the class, next to the same
narrowing already present for `filter`. That is an annotation only, with no
runtime effect.
Providers that do not currently follow the documented rules are deliberately
left alone; they are tracked separately rather than fixed here.
### TESTING INSTRUCTIONS
This change is documentation plus a behavior-preserving extraction, so the
test of record is that the existing semantic-layer behavior is unchanged and
the extraction is provably equivalent.
```bash
# Full semantic-layer unit suite, with the 100% coverage gate for that
package
pytest tests/unit_tests/semantic_layers/ \
--cov=superset/semantic_layers/ --cov-report=term-missing
--cov-fail-under=100
# The new helper's own tests, including the equivalence guard
pytest tests/unit_tests/semantic_layers/members_test.py
```
Expected: the semantic-layer suite passes (811 tests locally) with
`superset/semantic_layers` at 100% statement and branch coverage, and
`members_test.py` passes 5 tests. `test_matches_the_lookup_it_replaces` is the
one to read first: it asserts that `members_by_key` produces the same mapping
*and* the same key order as the dict comprehension it replaces, which is what
makes the three mapper substitutions safe.
To review the documentation itself, read the four docstrings against the
code they describe: `Metric`/`Dimension` and `Grains` in
`superset-core/src/superset_core/semantic_layers/types.py`, `Operator` and
`SemanticQuery` in the same file, and the `SemanticView` class docstring in
`view.py`.
The whole `tests/unit_tests` suite was also run: 22420 passed, with only
pre-existing environment failures unrelated to this change (two arm64
long-double tests and one translations script test that needs `python` on a
restricted `PATH`), each confirmed to fail identically on the base commit.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
The documented rules concern code reached only with the `SEMANTIC_LAYERS`
feature flag enabled, but nothing here changes flag behavior or adds a flag.
The `superset-core` changes are docstrings only, so the published SDK surface
is unchanged; `members_by_key` is an internal host helper, not a new public API.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
