sadpandajoe commented on code in PR #45855:
URL: https://github.com/apache/superset/pull/45855#discussion_r4448857691
##########
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:
Thanks for the review, and for being fine with leaving this one for a
follow-up. On the inspector-lookup question: I tried it and kept it out of this
PR.
The lookup itself is one `get_columns` call. The comparison that decides
"the column is still there" is where it breaks. Comparing names
case-insensitively treats a dataset column `Dt_Key` that was renamed to
`dt_key` at the source as present, even though the mirror quotes `"Dt_Key"` and
the warehouse rejects it, so the chart stops recovering. An exact-case
comparison fixes that, but it is still unreliable where the inspector does not
report the physical spelling. The Snowflake engine spec emits exact-case quoted
identifiers when columns are not normalized, while, as I read the Snowflake
SQLAlchemy dialect, the names it reflects are case-normalized, so a physical
`DT_KEY` that was renamed away can still look present.
A false "present" leaves a chart failing that used to recover. Without the
check the cost is one unpruned rerun after an error that only echoes the
column, and the docs now spell that case out. So I kept the current behavior. A
dialect-aware version, with per-engine identifier semantics, looks like the
right follow-up, and I'm happy to take it separately.
--
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]
