aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4392951413
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,156 @@ def get_rendered_sql(
def text(self, clause: str) -> TextClause:
return self.db_engine_spec.get_text_clause(clause)
+ def _get_rls_schema(self) -> str:
+ """Resolve RLS tables with the same schema policy as SQL
expressions."""
+ return get_rls_schema(
+ self.database,
+ self.catalog,
+ self.schema,
+ self.sql or "",
+ self.template_params_dict,
+ )
+
+ def _get_query_sql_expressions(
+ self, query_obj: QueryObjectDict, template_processor:
BaseTemplateProcessor
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ # Real column names take precedence over verbose names in filter
lookup.
+ columns = {col.verbose_name: col.expression for col in self.columns}
+ columns.update({col.column_name: col.expression for col in
self.columns})
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ expressions: list[str] = []
+
+ def add(value: Any, metric: bool = False) -> None:
+ """Resolve stored references in the namespace used by the query
sink."""
+ if isinstance(value, str):
+ references = metrics if metric else columns
+ expressions.append(references.get(value) or value)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ # Adhoc columns can name a saved calculated column after
+ # rendering. SQL metrics use their SQL literally instead.
+ expressions.append(
+ sql if "expressionType" in value else columns.get(sql)
or sql
+ )
+ add(value.get("column"))
+ add(value.get("column_name"))
+
+ for value in [
+ *(query_obj.get("columns") or []),
+ *(query_obj.get("groupby") or []),
+ ]:
+ add(value)
+ for metric_value in query_obj.get("metrics") or []:
+ add(metric_value, metric=True)
+ for orderby_value, _ascending in query_obj.get("orderby") or []:
+ add(
+ orderby_value,
+ metric=isinstance(orderby_value, str) and orderby_value in
metrics,
+ )
+ for flt in query_obj.get("filter") or []:
+ filter_column = flt.get("col")
+ add(
+ filter_column,
+ metric=isinstance(filter_column, str) and filter_column not in
columns,
+ )
+ series_limit = query_obj.get("series_limit") or (
+ query_obj.get("timeseries_limit")
+ if query_obj.get("is_timeseries", True)
+ else 0
+ )
+ add(
+ (
+ query_obj.get("series_limit_metric")
+ or query_obj.get("timeseries_limit_metric")
+ )
+ if series_limit
+ else None,
+ metric=True,
+ )
+ add(query_obj.get("granularity"))
+ add(self.main_dttm_col if query_obj.get("granularity") else None)
Review Comment:
My earlier reply wrongly listed `superset/models/helpers.py:5544` as not
affected: the series-limit join builds a time filter outside the removal check.
Fixed in bd37cdc56758ac2189e32972b1b1170204f2e923. The collector includes
that time column's rendered SQL/RLS when the join planner still uses it,
without undoing outer-filter removal, bare-name handling, single rendering, or
raw GROUP BY collection.
Regression: `test_removed_time_filter_series_limit_cache_matches_execution`
in `tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`
covers SqlaTable and Query, `ds` and `__timestamp` removal, join and both
prequery planners. With only helpers.py restored to
aa1813a84f789496e71aa86ed6bc8e5643dc06fb, four join cases fail: the two rule
sets produce `[1]` versus `[]`, but share a key. All 20 cases pass with the
change; 16 compatibility controls also pass before it. The tests assert exact
RLS keys, single source rendering and no chart-query build during collection.
Docker: 401 regression-module tests pass (including the earlier 381); 1,891
tests across connectors/models/schema parameters and 1,482 RLS/parser tests
pass. Changed-file pre-commit and mypy pass. Test containers were removed;
`docker ps -a --filter name=pr45681` returns only the header.
Siblings checked:
- superset/models/helpers.py:4196 — fixed in
bd37cdc56758ac2189e32972b1b1170204f2e923
- superset/models/helpers.py:6338 — fixed in
bd37cdc56758ac2189e32972b1b1170204f2e923
- superset/connectors/sqla/models.py:2933 — fixed in
bd37cdc56758ac2189e32972b1b1170204f2e923
- superset/models/sql_lab.py:425 — fixed in
bd37cdc56758ac2189e32972b1b1170204f2e923
- superset/models/helpers.py:6435 — not affected: no-join/no-subquery
prequeries rebuild a normal query and honor template removal; both paths have
two-user regression controls.
- superset/models/helpers.py:6337 — not affected: time_groupby_inline
engines skip the inner time filter; covered by the inline planner control.
- superset/models/helpers.py:5589 — not affected: the primary-time predicate
checks removal before rendering; existing removed-primary tests pass.
- superset/models/helpers.py:5606 — not affected: the outer time predicate
checks both the time-column name and __timestamp before rendering; existing
removed-time tests pass.
- superset/models/helpers.py:5790 — not affected: saved-column filters check
removal before rendering; existing ordinary/verbose removal tests pass.
- superset/models/helpers.py:5744 — not affected: adhoc/metric filters
render eagerly before removal and remain eagerly collected; existing
eager-rendering tests pass.
- superset/models/helpers.py:6241 — not affected: TEMPORAL_RANGE filter
construction is downstream of the same removal guard.
- superset/models/helpers.py:4630 — not affected: Others reconstruction
reuses accumulated time/column predicates rather than discovering another time
expression; the join builder uses the corrected collector.
- superset/jinja_context.py:728 — not affected: get_time_filter records
removals in the shared processor list.
- superset/jinja_context.py:672 — not affected: get_filters records column
removals in the same shared list.
- superset/common/query_context_processor.py:461 — not affected: cache-key
construction delegates to the datasource collector without separate expression
discovery.
- superset/tasks/async_queries.py:372 — not affected: async task keys
delegate through query_context.query_cache_key to the same datasource collector.
--
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]
