alexandrusoare commented on code in PR #44650:
URL: https://github.com/apache/superset/pull/44650#discussion_r4105025370
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2821,68 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. A correlated sub-query is keyed to the rows of an enclosing query,
like a
+ join, and the body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output
like a
+ join, so both are left out.
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
+ ]
+ while pending:
+ scope = pending.pop()
+ if id(scope) in found:
+ continue
+ found.add(id(scope))
+ pending.extend(child for child in scopes if child.parent is scope)
+ pending.extend(
+ source
+ for source in scope.sources.values()
+ if isinstance(source, Scope) and source.scope_type == ScopeType.CTE
+ )
+ return found
+
+
+def _is_correlated(scope: Scope) -> bool:
+ """
+ Does a sub-query reference a table of an enclosing query?
+
+ Only a column qualified with an enclosing table's name or alias counts,
when the
+ sub-query has no table of its own under that name. An unqualified column
can't
+ be told apart from one of the sub-query's own, so it is treated as local,
which
+ errs toward the sub-query getting the stricter rules. (``Scope``'s own
+ ``is_correlated_subquery`` treats every unqualified column as external.)
+
+ :param scope: A ``SUBQUERY`` scope
+ :returns: True if the sub-query is correlated
+ """
+ enclosing: set[str] = set()
+ parent = scope.parent
+ while parent:
+ enclosing.update(parent.sources)
+ parent = parent.parent
+ return any(
+ column.table in enclosing and column.table not in scope.sources
+ for column in scope.columns
Review Comment:
`scope.columns` includes a nested sub-query's columns, so an uncorrelated
sub-query wrapping a correlated one gets flagged correlated and loses the rules.
##########
superset/utils/rls.py:
##########
@@ -83,24 +86,40 @@ def apply_rls(
# collect all RLS predicates for all tables in the query
default_catalog = database.get_default_catalog()
- predicates: dict[Table, list[Any]] = {}
- for table in parsed_statement.tables:
- table = table.qualify(catalog=catalog, schema=schema)
- predicates[table] = [
- parsed_statement.parse_predicate(predicate)
- for predicate in get_predicates_for_table(
- table,
- database,
- default_catalog,
- exclude_dataset_id=exclude_dataset_id,
- include_global_guest_rls=include_global_guest_rls,
- )
- if predicate
- ]
-
- has_predicates = any(predicates.values())
- parsed_statement.apply_rls(catalog, schema, predicates, method)
- return has_predicates
+
+ def collect_predicates(include_global: bool) -> dict[Table, list[Any]]:
+ predicates: dict[Table, list[Any]] = {}
+ for table in parsed_statement.tables:
+ table = table.qualify(catalog=catalog, schema=schema)
+ predicates[table] = [
+ parsed_statement.parse_predicate(predicate)
+ for predicate in get_predicates_for_table(
+ table,
+ database,
+ default_catalog,
+ exclude_dataset_id=exclude_dataset_id,
Review Comment:
`exclude_dataset_id` rests on the same outer-query argument as the global
guest rules — should sub-query reads drop it too?
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2821,68 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. A correlated sub-query is keyed to the rows of an enclosing query,
like a
+ join, and the body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output
like a
+ join, so both are left out.
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
Review Comment:
A correlated scalar sub-query isn't like a join — `(SELECT COUNT(*) FROM b
WHERE b.a_id = a.id)` never sends b's rows to the output, so the outer filter
can't narrow `n`; is dropping the rules there intended?
--
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]
