Skip to content

Commit 83f4957

Browse files
AryaKetanShCtclaude
andcommitted
fix: exclude the DataFrame parameter from the supported options
Review comment on apache#42927. `exec_post_processing` calls the operation as `operation(df, **options)`, so the first parameter takes the DataFrame positionally. The name check accepted every parameter of the signature, therefore an option named `df` counted as supported and reached the call, which then raised `TypeError: pivot() got multiple values for argument 'df'`. The behaviour is the same before this pull request, because the options went to the operation unchanged. The check must still not call such an option supported. It now compares against the parameters that a caller can give by keyword: the first parameter and any positional-only parameter are excluded. Also adds tests for the branches that the first commit left uncovered: an option named `df`, an operation that takes `**kwargs`, and an entry that names no operation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 3982225 commit 83f4957

2 files changed

Lines changed: 61 additions & 2 deletions

File tree

superset/common/query_object.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,8 +248,17 @@ def _drop_unsupported_options(post_proc: dict[str, Any]) -> dict[str, Any]:
248248
):
249249
return post_proc
250250

251+
# `exec_post_processing` calls the operation as `operation(df, **options)`,
252+
# so the first parameter receives the DataFrame positionally and can never
253+
# be supplied as an option, and neither can a positional-only parameter.
254+
keyword_parameters = {
255+
name
256+
for position, (name, parameter) in enumerate(parameters.items())
257+
if position > 0 and parameter.kind is not inspect.Parameter.POSITIONAL_ONLY
258+
}
259+
251260
options = post_proc.get("options") or {}
252-
unsupported = {key for key in options if key not in parameters}
261+
unsupported = {key for key in options if key not in keyword_parameters}
253262
if not unsupported:
254263
return post_proc
255264

@@ -263,7 +272,9 @@ def _drop_unsupported_options(post_proc: dict[str, Any]) -> dict[str, Any]:
263272
return {
264273
**post_proc,
265274
"options": {
266-
key: value for key, value in options.items() if key in parameters
275+
key: value
276+
for key, value in options.items()
277+
if key in keyword_parameters
267278
},
268279
}
269280

tests/unit_tests/queries/query_object_test.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from superset.common.query_object import QueryObject
2222
from superset.connectors.sqla.models import SqlaTable
2323
from superset.models.core import Database
24+
from superset.utils import pandas_postprocessing
2425
from superset.utils.core import override_user
2526

2627

@@ -433,3 +434,50 @@ def test_post_processing_keeps_unknown_operation():
433434
assert query_object.post_processing == [
434435
{"operation": "does_not_exist", "options": {"a": 1}}
435436
]
437+
438+
439+
def test_post_processing_drops_the_dataframe_parameter():
440+
"""
441+
The DataFrame parameter is not an option.
442+
443+
`exec_post_processing` calls `operation(df, **options)`, so an option named
444+
after the first parameter would raise `TypeError: pivot() got multiple
445+
values for argument 'df'`.
446+
"""
447+
query_object = QueryObject(
448+
row_limit=1,
449+
post_processing=[
450+
{
451+
"operation": "pivot",
452+
"options": {"df": "malformed", "index": ["a"], "aggregates": {}},
453+
}
454+
],
455+
)
456+
457+
options = query_object.post_processing[0]["options"]
458+
assert "df" not in options
459+
assert options["index"] == ["a"]
460+
461+
462+
def test_post_processing_keeps_options_of_a_variadic_operation():
463+
"""An operation that accepts `**kwargs` accepts every option."""
464+
465+
def variadic(df, **kwargs):
466+
return df
467+
468+
post_processing = [{"operation": "variadic", "options": {"anything": 1}}]
469+
with patch.object(pandas_postprocessing, "variadic", variadic, create=True):
470+
query_object = QueryObject(row_limit=1, post_processing=post_processing)
471+
472+
assert query_object.post_processing == post_processing
473+
474+
475+
def test_post_processing_keeps_an_entry_without_an_operation():
476+
"""
477+
An entry that names no operation is kept, so that `exec_post_processing`
478+
reports it as an `InvalidPostProcessingError`.
479+
"""
480+
post_processing = [{"options": {"a": 1}}]
481+
query_object = QueryObject(row_limit=1, post_processing=post_processing)
482+
483+
assert query_object.post_processing == post_processing

0 commit comments

Comments
 (0)