rebenitez1802 commented on PR #43829:
URL: https://github.com/apache/superset/pull/43829#issuecomment-5541014281
Thanks for the thorough review, Amin — especially reverting the production
lines to confirm the tests fail without them.
**On the question (is the narrowing intended?)** — Yes, this is intended. A
schema is a dataset-only attribute: semantic views carry
`literal(None).label("schema")`, and the Schema dropdown is sourced only from
dataset schemas (`/api/v1/dataset/distinct/schema`). Under AND semantics, "show
me things in schema X" cannot match a schema-less semantic view, so excluding
views is the honest result. The alternative — filter the dataset leg but let
the view leg pass through — would surface rows that don't match the active
filter, i.e. the exact bug class this PR fixes. It's also consistent with the
existing **Database** connection filter, which already narrows the combined
list to datasets the same way, so this isn't a new pattern — just the same rule
applied to a second dataset-only attribute.
**On the Type="Semantic View" + schema nit** — agreed, and I've taken your
suggestion in `cb526a2`. `_resolve_source_type` now returns `"empty"` for that
pair (semantic-views-in-schema-X is an empty set under AND) instead of silently
returning datasets, pinned by
`test_resolve_source_type_semantic_view_type_plus_schema_is_empty`. As you
noted, only the schema + type case needs a guard — `sql_filter` and
`type_filter` share one control and can't collide — so that's the only
combination special-cased. This makes the precedence intentional rather than
incidental. Left the frontend cascade untouched; the backend now gives the
honest answer regardless of how the combination is reached.
(Heads-up: I pushed `cb526a2` on top of the master merge you approved, so
it's a commit past your approval SHA — a fresh 👍 whenever you get a moment
would be appreciated.)
--
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]
