AryaKetanShCt commented on code in PR #42927:
URL: https://github.com/apache/superset/pull/42927#discussion_r3746810885
##########
superset/common/query_object.py:
##########
@@ -205,8 +206,77 @@ def is_str_or_adhoc(metric: Metric) -> bool:
def _set_post_processing(
self, post_processing: list[dict[str, Any] | None] | None
) -> None:
- post_processing = post_processing or []
- self.post_processing = [post_proc for post_proc in post_processing if
post_proc]
+ self.post_processing = [
+ self._drop_unsupported_options(post_proc)
+ for post_proc in post_processing or []
+ if post_proc
+ ]
+
+ @staticmethod
+ def _drop_unsupported_options(post_proc: dict[str, Any]) -> dict[str, Any]:
+ """
+ Drop options that the post-processing operation no longer accepts.
+
+ A chart's ``query_context`` is written when the chart is saved and is
+ never rewritten afterwards, while Explore rebuilds the query from
+ ``form_data`` at every render. A chart saved by an older version of
+ Superset can therefore reference an option that has since been removed
+ from the operation. ``exec_post_processing`` passes the stored options
+ as keyword arguments, so that option raises a bare ``TypeError`` on
+ every path that replays the stored ``query_context`` -- the chart data
+ endpoint, alerts and reports, thumbnails, CSV export -- while the same
+ chart still renders correctly in Explore.
+
+ Comparing against the signature avoids a hard-coded list of removed
+ option names, which would need extending at each release.
+ """
+ operation = post_proc.get("operation")
+ function = (
+ getattr(pandas_postprocessing, operation, None)
+ if isinstance(operation, str)
+ else None
+ )
+ if function is None:
+ # A missing or unknown operation is left untouched, so that
+ # exec_post_processing reports it as InvalidPostProcessingError.
+ return post_proc
+
+ parameters = inspect.signature(function).parameters
+ if any(
+ parameter.kind is inspect.Parameter.VAR_KEYWORD
+ for parameter in parameters.values()
+ ):
+ return post_proc
+
+ # `exec_post_processing` calls the operation as `operation(df,
**options)`,
+ # so the first parameter receives the DataFrame positionally and can
never
+ # be supplied as an option, and neither can a positional-only
parameter.
+ keyword_parameters = {
+ name
+ for position, (name, parameter) in enumerate(parameters.items())
+ if position > 0 and parameter.kind is not
inspect.Parameter.POSITIONAL_ONLY
+ }
Review Comment:
Correct, and fixed in `781c6c4`.
`keyword_parameters` now only holds the kinds a caller can actually fill by
keyword:
```python
keyword_parameters = {
name
for position, (name, parameter) in enumerate(parameters.items())
if position > 0
and parameter.kind
in (
inspect.Parameter.POSITIONAL_OR_KEYWORD,
inspect.Parameter.KEYWORD_ONLY,
)
}
```
That drops `VAR_POSITIONAL` along with the positional-only parameters the
earlier check already excluded. The `VAR_KEYWORD` early return above stays as
it is, because an operation taking `**kwargs` really does accept every option
and nothing should be filtered for it.
No operation in `superset/utils/pandas_postprocessing` takes `*args` today,
so this is a correctness fix to the check rather than a fix to an observed
failure. Covered by `test_post_processing_drops_a_variadic_positional_option`.
##########
superset/common/query_object.py:
##########
@@ -205,8 +206,77 @@ def is_str_or_adhoc(metric: Metric) -> bool:
def _set_post_processing(
self, post_processing: list[dict[str, Any] | None] | None
) -> None:
- post_processing = post_processing or []
- self.post_processing = [post_proc for post_proc in post_processing if
post_proc]
+ self.post_processing = [
+ self._drop_unsupported_options(post_proc)
+ for post_proc in post_processing or []
+ if post_proc
+ ]
+
+ @staticmethod
+ def _drop_unsupported_options(post_proc: dict[str, Any]) -> dict[str, Any]:
+ """
+ Drop options that the post-processing operation no longer accepts.
+
+ A chart's ``query_context`` is written when the chart is saved and is
+ never rewritten afterwards, while Explore rebuilds the query from
+ ``form_data`` at every render. A chart saved by an older version of
+ Superset can therefore reference an option that has since been removed
+ from the operation. ``exec_post_processing`` passes the stored options
+ as keyword arguments, so that option raises a bare ``TypeError`` on
+ every path that replays the stored ``query_context`` -- the chart data
+ endpoint, alerts and reports, thumbnails, CSV export -- while the same
+ chart still renders correctly in Explore.
+
+ Comparing against the signature avoids a hard-coded list of removed
+ option names, which would need extending at each release.
+ """
+ operation = post_proc.get("operation")
+ function = (
+ getattr(pandas_postprocessing, operation, None)
+ if isinstance(operation, str)
+ else None
+ )
+ if function is None:
+ # A missing or unknown operation is left untouched, so that
+ # exec_post_processing reports it as InvalidPostProcessingError.
+ return post_proc
+
+ parameters = inspect.signature(function).parameters
+ if any(
+ parameter.kind is inspect.Parameter.VAR_KEYWORD
+ for parameter in parameters.values()
+ ):
+ return post_proc
+
+ # `exec_post_processing` calls the operation as `operation(df,
**options)`,
+ # so the first parameter receives the DataFrame positionally and can
never
+ # be supplied as an option, and neither can a positional-only
parameter.
+ keyword_parameters = {
+ name
+ for position, (name, parameter) in enumerate(parameters.items())
+ if position > 0 and parameter.kind is not
inspect.Parameter.POSITIONAL_ONLY
+ }
+
+ options = post_proc.get("options") or {}
+ unsupported = {key for key in options if key not in keyword_parameters}
+ if not unsupported:
+ return post_proc
+
+ logger.warning(
+ "Dropping unsupported option(s) %s of post-processing operation "
+ "`%s`. The chart's stored query_context predates the current "
+ "signature of that operation.",
+ sorted(unsupported),
+ operation,
+ )
Review Comment:
Agreed on the noise, lowered to `logger.info` in `781c6c4`.
A chart saved before the option was removed reaches this on every render, so
the message repeats for as long as the chart is not resaved and never reports
anything new. That is a poor fit for `warning`.
Left the deduplication cache out. The message is emitted once per operation
per request, only for charts with a stale `query_context`, and a per-process
cache keyed on `(operation, frozenset(unsupported))` would hide the recurrence
that tells an operator the chart is still stale. If the volume turns out to be
a problem in practice, the cheaper next step is a `logging` filter on this
module rather than state carried inside `QueryObject`.
--
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]
