fix: drop post-processing options the operation no longer accepts - #42927
Conversation
Code Review Agent Run #833b8fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The To resolve this, you should explicitly exclude Proposed FixIn options = post_proc.get("options") or {}
unsupported = {key for key in options if key not in parameters or key == "df"}
if not unsupported:
return post_procThis change ensures that even if Would you like me to check the rest of the comments on this PR to see if there are other issues that need addressing? superset/common/query_object.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42927 +/- ##
==========================================
- Coverage 66.73% 66.73% -0.01%
==========================================
Files 2876 2876
Lines 164201 164218 +17
Branches 37887 37890 +3
==========================================
+ Hits 109577 109587 +10
- Misses 52467 52471 +4
- Partials 2157 2160 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
|
Thanks — the One clarification for the record: the same call fails on The check now compares against the parameters that a caller can give by keyword. It excludes the first parameter, which takes the DataFrame positionally in keyword_parameters = {
name
for position, (name, parameter) in enumerate(parameters.items())
if position > 0 and parameter.kind is not inspect.Parameter.POSITIONAL_ONLY
}An option named The commit also adds tests for the three branches that the first commit left uncovered, which was the codecov report: an option named |
Code Review Agent Run #b871e5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds backward-compatible handling for stored query_context post-processing options by filtering out options no longer accepted by pandas post-processing operations.
Changes:
- Filter unsupported post-processing
optionsinQueryObjectby inspecting the target operation’s signature. - Preserve decorated operation signatures via
functools.wrapsso signature inspection remains accurate. - Add unit tests covering option dropping behavior and decorator signature preservation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit_tests/queries/query_object_test.py | Adds tests for dropping unsupported post-processing options while retaining valid/unknown operations. |
| tests/unit_tests/pandas_postprocessing/test_utils.py | Adds regression test ensuring decorated operations keep their original signature for inspect.signature. |
| superset/utils/pandas_postprocessing/utils.py | Updates decorator to use wraps so signature/name are preserved. |
| superset/common/query_object.py | Implements signature-based filtering of unsupported post-processing options and logs when dropping occurs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A chart's `query_context` is written when the chart is saved and is never rewritten. Explore rebuilds the query from `form_data` at every render and never reads it, so only the paths that are not a browser replay it: the chart data endpoint, alerts and reports, thumbnails, cache warm-up and CSV export. The stored query therefore ages while the engine moves on. `pivot` used to accept `flatten_columns` and `reset_index`; flattening became its own operation and the parameters were removed. `exec_post_processing` passes the stored options as keyword arguments, so replaying a chart saved before that change raises `TypeError: pivot() got an unexpected keyword argument 'flatten_columns'` on every one of those paths, while the same chart still renders correctly in Explore. There is no migration for the stored query, so the failure is permanent until somebody re-saves each chart by hand. `QueryObject` now compares the stored options against the signature of the operation and drops the ones it no longer accepts, with a warning naming the operation and the options. Comparing against the signature avoids a list of removed option names that would need extending at each release. An operation that takes `**kwargs` is left alone, and an unknown operation is left for `exec_post_processing` to report as `InvalidPostProcessingError`. That comparison needs a signature to read. `validate_column_args` returned `def wrapped(df, **options)` without `functools.wraps`, so all ten operations that use it reported `(df, **options)` and lost their `__name__` and `__doc__`. `inspect.unwrap` could not recover the original, because without `wraps` there is no `__wrapped__`. Adding `wraps` restores the signature, the name and the docstring. Fixes the first symptom of apache#42926. The second symptom in that issue, a stored query that sets `is_timeseries` without a temporal column, is left out on purpose: `_apply_granularity` has since gained its own inference path, and the intended behaviour there deserves a maintainer's opinion first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
83f4957 to
4495fa0
Compare
`exec_post_processing` calls the operation as `operation(df, **options)`, so an option can only reach a parameter that accepts a keyword argument. The check also accepted `*args`, which cannot be filled that way, so an option named after it was reported as supported and still raised `TypeError: got an unexpected keyword argument`. Restrict the supported set to POSITIONAL_OR_KEYWORD and KEYWORD_ONLY parameters, which drops `*args` alongside the positional-only parameters already excluded. Log the dropped options at info rather than warning. A chart saved before an option was removed reaches this on every render, so a warning repeats for as long as the chart is not resaved without reporting anything new. Signed-off-by: Arya Ketan <aryaketan@sharechat.co>
|
hi @rusackas , can u pl re-trigger the workflow, |
Code Review Agent Run #21d3e2Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Signed-off-by: Arya Ketan <aryaketan@sharechat.co> # Conflicts: # tests/unit_tests/queries/query_object_test.py
|
@msyavuz @rusackas — this one is ready for review whenever you have a moment. Updated to State:
No rush on my end, and happy to split or reshape it if the scope is awkward to review as one change. |
|
Thanks @AryaKetanShCt, LGTM! Nice diagnosis on the stale |
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Fixes the first symptom of #42926.
A chart's
query_contextis written when the chart is saved and is never rewritten. Explore rebuilds the query fromform_dataat every render and never reads it, so only the paths that are not a browser replay it:GET /api/v1/chart/<id>/data/, alerts and reports, thumbnails, cache warm-up, CSV export.The stored query therefore ages while the engine moves on.
pivotused to acceptflatten_columnsandreset_index; flattening became its own operation and those parameters were removed.exec_post_processingpasses the stored options straight through:so replaying a chart saved before that change gives
on every one of those paths, while the same chart renders correctly in Explore. There is no migration for the stored query, so the failure is permanent until somebody opens each chart and re-saves it.
The change.
QueryObjectcompares the stored options against the signature of the operation and drops those it no longer accepts, logging a warning that names the operation and the options. Comparing against the signature avoids a hard-coded list of removed names, which would need extending at each release. An operation that takes**kwargsis left alone. An unknown operation is left in place so thatexec_post_processingstill reports it asInvalidPostProcessingError.Why
functools.wrapsis in the same PR. That comparison needs a signature to read, and there was none.validate_column_argsreturneddef wrapped(df, **options)withoutwraps:All ten operations using that decorator (
aggregate,compare,contribution,cum,diff,pivot,rename,rolling,select,sort) reported**kwargsand lost their name and docstring.inspect.unwrapcannot recover the original, because withoutwrapsthere is no__wrapped__. Addingwrapsrestores the signature, the name and the docstring. Happy to split this into its own PR if you prefer.Scope. The second symptom in #42926 — a stored query that sets
is_timeserieswithout a temporal column — is deliberately not addressed here._apply_granularityhas since gained its own inference path, so the intended behaviour there deserves a maintainer's opinion before I send code.Behaviour for current charts is unchanged. A
query_contextbuilt by the current frontend has options that match the current signature, so nothing is dropped and the same dict object is returned.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Server-side; no UI change.
Before, on a chart saved in 2023:
After, the same chart returns its rows. The chart itself was never broken in Explore, before or after.
TESTING INSTRUCTIONS
New unit tests:
tests/unit_tests/queries/query_object_test.pypivotwithflatten_columnsandreset_indexkeeps only the supported options, and keeps their valuesquery_contextis returned unchanged, as the same objectexec_post_processingto rejecttests/unit_tests/pandas_postprocessing/test_utils.pyManual check on any version:
This change is also running in production on a 6.1.0 instance, applied as a patch. Three charts that had failed for years — an
echarts_timeseries_barand abig_numbersaved in 2023 — now return data through the chart-data endpoint, and charts saved recently are unaffected.ADDITIONAL INFORMATION