sadpandajoe commented on code in PR #45855:
URL: https://github.com/apache/superset/pull/45855#discussion_r4446408132
##########
docs/admin_docs/configuration/partition-filter-mapping.mdx:
##########
@@ -263,6 +263,44 @@ The mirror only has to be *no narrower* than the filter it
stands in for, so adm
`NULL` partition costs one extra partition read and keeps those rows. Engines
still prune
everything else.
+## When the partition column is removed from the warehouse
+
+Superset reads the partition column from the dataset's stored column list, so
a partition
+column that is dropped or renamed in the warehouse is still mirrored until the
dataset's
+columns are refreshed. A column that a calculated partition column's
expression reads counts
+the same way.
+
+When the warehouse rejects a chart query because of that column, Superset runs
the query once
+more without the mirrored predicate. The rows returned are the same; the query
is slower,
+because it reads every partition. The unpruned result is cached like any other.
+
+This applies to chart queries run through the normal chart path, and the cost
depends on the
+query:
+
+- A simple query that is rejected makes two attempts: the rejected one and the
unpruned run.
+- A chart with a series limit that runs its ranking as a separate query can
make more
+ attempts, for example six statements instead of two, and a time comparison
repeats the
+ rejected attempt and the retry for each comparison period.
+- Streaming exports and the cursor-paginated Samples pages do not use this
recovery.
+
+Limits:
+
+- The retry happens when the warehouse's error message names the column, or a
column the
+ calculated expression reads, whatever the underlying cause. A message that
names neither
+ (for instance one that names only a function in the expression) is not
retried, and the
+ chart fails as it would without this recovery. Conversely, an error that
merely echoes the
+ column, such as a driver timeout that repeats the SQL, can also trigger the
unpruned run.
+ Errors raised by Superset itself, such as its own timeouts, permission
checks, and
+ cancellations, are never retried.
+- Removing the mirror cannot rescue a query that itself selects the missing
column; the
+ warehouse's error is shown. This includes Samples and Drill to detail, which
select every
+ column of the dataset, and any chart or table that uses the column.
+- The pruning indicator in the dataset editor and on filters stays on, because
it reads the
Review Comment:
Fixed in 54e8161c7a — the docs now list View query next to the pruning
indicator: both keep showing the mirrored predicate after a fallback, because
View query shows the statement Superset would send first, not the unpruned one
that ran. It does not execute the main chart statement, though building it can
run transform probes and ranking queries. The testing step in the PR
description is corrected to match.
##########
superset/models/helpers.py:
##########
@@ -2752,6 +2773,96 @@ def run_query(query_sql: str) -> Optional[pd.DataFrame]:
sql_shifted_temporal_labels=query_str_ext.sql_shifted_temporal_labels,
)
+ def _run_recovering_partition_mirror(
+ self,
+ query_obj: QueryObjectDict,
+ executed: list[QueryStringExtended],
+ run_query: Callable[[str], Optional[pd.DataFrame]],
+ ) -> Optional[pd.DataFrame]:
+ """
+ Run the statement in ``executed[-1]``, once more without its partition
+ filter mirror if the warehouse rejected the mirror.
+
+ Used for every statement except system-authored sampling of a physical
+ table (the Samples tab), which has its own retry and does not get this
+ one.
+
+ The unmirrored statement is appended to ``executed`` before it runs, so
+ a caller sees what was last sent even when that attempt fails too. A
+ failure that is not attributed to the mirror is re-raised unchanged; if
+ the retry itself fails, that failure is the one raised.
+ """
+ try:
+ return run_query(executed[-1].sql)
+ except Exception as failure: # pylint: disable=broad-except
+ unmirrored = self._query_without_rejected_partition_mirror(
+ query_obj, executed[-1], failure
+ )
+ if unmirrored is None:
+ raise
+ logger.warning(
+ "The warehouse rejected the partition filter mirror; retrying "
+ "without it: %s",
+ utils.error_msg_from_exception(failure),
+ )
+ executed.append(unmirrored)
Review Comment:
Fixed in 85ad34ca75 — the retried statement now goes through
`_raise_for_disallowed_sql` before it is recorded or executed, so a denylisted
table or function that only the fresh render carries is refused the same way as
in the first statement. The call is in `_run_recovering_partition_mirror`
rather than in the decision method, which is documented to never raise. Tests
(4fef9f21fe): a denied table and a denied function in the retried statement are
each refused with only one statement reaching the warehouse, plus a control
showing an allowed retry still recovers. A sentence in the docs says the same
(058d32cc4d).
--
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]
