EnxDev commented on code in PR #45855:
URL: https://github.com/apache/superset/pull/45855#discussion_r4431282373
##########
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:
View query builds its SQL through `_get_query`, which keeps the mirror and
never runs anything, so after a fallback it still shows the `dt_key >= ...`
statement, not the one that ran.
That's also why step 3 of the testing instructions won't hold. Could we list
View query here next to the pruning indicator?
##########
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)
+ return run_query(unmirrored.sql)
+
+ def _query_without_rejected_partition_mirror(
+ self,
+ query_obj: QueryObjectDict,
+ query_str: QueryStringExtended,
+ failure: Exception,
+ ) -> Optional[QueryStringExtended]:
+ """
+ The statement to run instead of one the warehouse rejected because of
+ its partition filter mirror, or ``None`` when the failure is not that.
+
+ A mirror is an optimisation the original predicates already cover, so
+ dropping it returns the same rows (given the invariant the mapping
+ assumes), only slower. The resolver reads the
+ dataset's cached columns, which still list a partition column that was
+ dropped or renamed at the source, so the warehouse can reject a
+ statement for a column the chart never selects.
+
+ Only a warehouse failure is attributed to the mirror: a raw driver
+ error, or a ``SupersetDBAPIError`` an engine spec mapped one into.
+ Everything else Superset raises on this path -- its own timeouts,
+ security, cancellation, validation and OAuth errors -- is control flow
+ and is never retried, as is a ``BaseException`` such as a worker
+ deadline. A warehouse failure is retried when its message names an
+ identifier the mirror emitted, whatever the underlying cause: a driver
+ timeout, a resource limit or a permission error whose message does not
+ mention those identifiers is left alone, but one that does -- by
+ echoing the SQL, say -- also gets the unpruned run.
+
+ Never raises: anything that goes wrong while deciding leaves the
+ original failure to surface.
+ """
+ if isinstance(failure, SupersetException) and not isinstance(
+ failure, SupersetDBAPIError
+ ):
+ return None
+ try:
+ mapping = resolve_partition_mapping(cast("SqlaTable", self))
+ if mapping is None or not failure_names_mirror_identifier(
Review Comment:
Matching on the message also catches errors that only echo the SQL (the docs
call this out), and then the retry is the unpruned statement: a query that just
timed out with pruning gets rerun reading every partition.
Could the retry first confirm the column is actually missing from the source
table, with one inspector column lookup on this failure path only, before going
unpruned?
##########
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:
The retried statement never goes through `_raise_for_disallowed_sql`. It's a
fresh render, and Jinja's `random` filter means it can differ from the one that
was checked, so a chart editor with template processing on could get a
denylisted table into the retry.
Cheap to close:
```suggestion
self._raise_for_disallowed_sql(unmirrored.sql)
executed.append(unmirrored)
```
--
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]
