aminghadersohi commented on code in PR #45108:
URL: https://github.com/apache/superset/pull/45108#discussion_r4254504507
##########
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:
Only name lookups keep the last one: grouping in `_map_query_object` keeps
the first matching `(name, grain)` variant, while filters and order resolve via
`members_by_key` (last), so a duplicate can group by one ID and filter on
another.
```suggestion
The host does not detect such a conflict: grouping keeps the first
variant
it sees for a ``(name, grain)`` pair, while filters and ordering resolve
the
name to the last variant seen, so one query can group and filter on
different IDs.
```
##########
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:
Same correction here: the selected variant is the first one seen, the
filtered one the last.
```suggestion
detect a duplicate ``(name, grain)`` (grouping and filtering may then
pick
different variants) or a metric name that collides with a dimension name.
A query selects at most one
```
--
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]
