mikebridge commented on code in PR #45108: URL: https://github.com/apache/superset/pull/45108#discussion_r4254651259
########## superset-core/src/superset_core/semantic_layers/types.py: ########## @@ -52,7 +52,46 @@ def __hash__(self) -> int: class Grains: - """Pre-defined common grains and factory for custom ones.""" + """ + Pre-defined common grains and factory for custom ones. + + **Grain-variant selection.** A view may expose several :class:`Dimension` + variants that share one ``name``, one per supported grain, optionally + alongside a raw variant whose ``grain`` is ``None``. When a selection does + not name an explicit, supported grouping grain, one variant has to be + chosen, and the rule is a single default preference: + + 1. the raw variant (``grain is None``); + 2. then, finest known grain first: second, minute, hour, day, week, month, + quarter, year; + 3. then any remaining grain representation, in lexical order. + + This describes the intended contract. Resolving every host lookup through + this one preference is the work of apache/superset#44454; until it lands, the + host does not apply it everywhere. Filter resolution keeps whichever variant + of a name it sees last, and ``get_dimensions`` returns a set, so that choice + is arbitrary; the grouping fallback orders the remaining grains by name. A + provider should not rely on which variant a given lookup picks today. + + Once that work lands, the preference governs column metadata, filters and the + grouping fallback. An explicit supported grouping grain is always honored. + Filters and time bounds will use the default preference **independently of + grouping**: grouping by month with a lower bound of January 15 will still + filter the raw time dimension from January 15 rather than testing month + buckets against that bound. Sorting and series limits use the selected + grouping grain. + + Do not order grains by ``Grain.name``: alphabetically "Day" precedes "Hour" + and "Quarter" precedes "Week", which is the opposite of grain fineness. Use + the preference above, keyed on ``representation``. + + Providers must expose at most one dimension ID per ``(name, grain)``. + Conflicting IDs, types, definitions or descriptions for the same name and + grain make the catalog ambiguous; that is a provider defect to fix rather + than a tie for the host to break by picking a variant. + The host does not detect such a conflict: it keeps the last variant it sees + for a ``(name, grain)`` pair. Review Comment: Good catch—applied your suggestion verbatim in 550b69fc207b487c90c9be3b4ba00f3020442d83. Verified against master 1578b8cbc1: grouping retains the first matching variant, while the name maps used by filters and ordering retain the last. This branch’s members_by_key preserves that lookup behavior. #44454 is still open, so the wording describes master rather than assuming its pending determinism changes have landed. Implemented and verified with Codex; this docstring-only follow-up used the dispatcher’s no-extra-review instruction (no new Claude review). Changed-file hooks and the normal pre-push gate passed. ########## superset-core/src/superset_core/semantic_layers/types.py: ########## @@ -84,6 +123,42 @@ def get(cls, representation: str, name: str | None = None) -> Grain: @dataclass(frozen=True) class Dimension: + """ + A groupable or filterable member of a semantic view. + + Member identity (the same rule applies to :class:`Metric`): + + - ``name`` is the **stable selection key**. Saved charts, API payloads and + compatibility selections reference a member by ``name``, so it must stay + stable across catalog reloads and provider upgrades. A provider that + changes what ``name`` means must declare a new + ``SemanticView.selection_identity_version`` so the host can ask the user + to reselect instead of failing with a generic "must be defined" error. + - ``verbose_name`` is **display only**, and is excluded from equality. + - ``id`` is **provider-private**. The host does not read it; do not expect a + selection, filter or order entry to round-trip through it. + + A view may expose several variants sharing one ``name``, one per supported + ``grain`` (see :class:`Grains` for how the host chooses between them), but + at most one per ``(name, grain)`` pair: two dimensions sharing a name and + grain are a provider defect, not a tie for the host to break silently. + Metric names are unique on their own, and must not collide with a dimension + name. Both rules are provider obligations: the host does not currently + detect a duplicate ``(name, grain)`` (it keeps the last variant it sees) or a + metric name that collides with a dimension name. A query selects at most one Review Comment: Applied this correction verbatim in 550b69fc207b487c90c9be3b4ba00f3020442d83 as well. Both descriptions now distinguish first-match grouping from last-match name lookup; no runtime behavior changed. #44454 remains a pending dependency rather than behavior claimed here. Implemented and verified with Codex; no new Claude review was required for this docs-only follow-up. Hooks and the normal pre-push gate passed. -- 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]
