aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4391936010
##########
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:
The remaining `remove_filter=True` case is closed in
bac4307d0cd0444a7f5883a80aedf49582984fbe. The collector renders the dataset
template with shared removal state before collecting delayed time/filter
expressions, matching execution. This covers the primary time column,
granularity, `__timestamp`, and ordinary/verbose filter names. Selected
expressions and eagerly rendered adhoc/metric filters remain collected.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_cache_key_skips_removed_primary_time_column`
- `test_cache_key_skips_removed_filter_column`
- `test_cache_key_skips_removed_time_filter_column`
All nine cases fail with `ZeroDivisionError` on
30e118ee16464af7d368c189404515d0c51b4c03 after execution succeeds, and pass at
the fixing commit. Additional regressions cover incomplete filters,
selected-column render counts, unused series-limit metrics, and the selected
timestamp pseudo-column. Reverting the source produces 21 failures; all 31
targeted cases pass with the fix, including 10 compatibility controls. Docker
validation: 346 affected-file tests and 3,348 broader tests passed (the broader
run includes the affected file, connectors, models, schema-resolution, RLS
utilities, and SQL parsing/metric normalization). Changed-file pre-commit
passed. Test containers were removed.
Siblings checked:
- superset/models/helpers.py:4094 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/models/helpers.py:4097 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/models/helpers.py:4127 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/models/helpers.py:4209 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/connectors/sqla/models.py:2871 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/models/sql_lab.py:395 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe
- superset/models/helpers.py:4103 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe (selected timestamp pseudo-column)
- superset/models/helpers.py:4131 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe (incomplete filters)
- superset/models/helpers.py:4149 — fixed in
bac4307d0cd0444a7f5883a80aedf49582984fbe (series metrics without matching
grouped series)
- superset/models/helpers.py:5544 — not affected: execution already honors
removed primary/time/filter columns; the collector mirrors those checks.
- superset/models/helpers.py:4087 — not affected: execution renders
adhoc/metric filter SQL eagerly, even for removed filters; compatibility tests
retain that behavior.
- superset/models/helpers.py:4137 — not affected: disabled fetch predicates
and empty WHERE/HAVING SQL stay skipped; selected aliases and overridden
grouping references retain their existing resolution rules.
- superset/jinja_context.py:563 — not affected: get_filters already records
removals; its processor receives the shared list.
- superset/jinja_context.py:728 — not affected: get_time_filter already
records removals; its processor receives the shared list.
- superset/common/query_context.py:157 — not affected: delegates to the
corrected datasource collector, including asynchronous callers through
superset/tasks/async_queries.py:372.
--
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]
