fix(query_object_factory): backport deprecated-field normalization to 6.0-bug-fixes (#41204)#41909
Conversation
…onstructing QueryObject (apache#41204) (cherry picked from commit 7040388)
|
Bito Automatic Review Skipped - Branch Excluded |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| for field in DEPRECATED_FIELDS: | ||
| if old_val := kwargs.pop(field.old_name, None): |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the newly introduced variables in the deprecated-field normalization block so the new logic is fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The added old_val local variable is introduced in new Python code and has no type annotation. This matches the rule requiring new or changed Python variables to include type hints.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_object_factory.py
**Line:** 71:72
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced variables in the deprecated-field normalization block so the new logic is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def test_deprecated_groupby_renamed_to_columns( | ||
| self, | ||
| query_object_factory: QueryObjectFactory, | ||
| raw_query_context: dict[str, Any], | ||
| ): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test method signature (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python test method and it lacks an explicit return type annotation. The custom rule requires new or changed Python functions and methods to include proper type hints, so -> None should be added.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_object_factory.py
**Line:** 142:146
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test method signature (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| for field in DEPRECATED_FIELDS: | ||
| if old_val := kwargs.pop(field.old_name, None): | ||
| kwargs.setdefault(field.new_name, old_val) |
There was a problem hiding this comment.
Suggestion: The deprecated-field normalization uses setdefault, which treats a key with value None as already populated. If a payload includes both a deprecated key (for example groupby) and a canonical key set to None (for example columns=None), the deprecated value is dropped after pop and never propagated, causing dimensions/limits to be lost. Update the merge logic to treat None (and any other "unset" sentinel you consider empty) as missing so valid deprecated values are preserved. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ /v1/query payloads can lose groupby dimensions.
- ❌ Affected charts render ungrouped or with incorrect aggregates.
- ⚠️ Stored query_contexts mixing keys may silently drop fields.Steps of Reproduction ✅
1. Craft a `query_context` JSON payload for the `/v1/query/` endpoint
(superset/views/api.py:60-17) where `queries[0]` includes both `"columns": null` and
`"groupby": ["name", "gender"]`, along with normal fields like `metrics`, `granularity`,
and `time_range`.
2. Submit this payload to `POST /v1/query/`; in `Api.query()` (superset/views/api.py:4-17)
the request body is deserialized and passed into
`QueryContextFactory.create(**json.loads(request.form["query_context"]))`.
3. Inside `QueryContextFactory.create()` (superset/common/query_context_factory.py:7-48),
each raw query dict is handed to `self._query_object_factory.create(...)` with
`**query_obj`, so `kwargs` in `QueryObjectFactory.create()`
(superset/common/query_object_factory.py:52-62) contains both `columns=None` and
`groupby=["name", "gender"]`.
4. In `QueryObjectFactory.create()` the deprecated-field normalization loop
(superset/common/query_object_factory.py:69-73) pops `groupby` into `old_val` and then
calls `kwargs.setdefault("columns", old_val)`. Because `columns` already exists with value
`None`, `setdefault` treats the key as populated and does not overwrite it, so `columns`
remains `None` and `groupby` is removed. When the `QueryObject` is constructed, `columns`
is interpreted as empty (`self.columns = columns or []` in
superset/common/query_object.py:142), causing the intended groupby dimensions to be lost
in downstream processing.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_object_factory.py
**Line:** 71:73
**Comment:**
*Incorrect Condition Logic: The deprecated-field normalization uses `setdefault`, which treats a key with value `None` as already populated. If a payload includes both a deprecated key (for example `groupby`) and a canonical key set to `None` (for example `columns=None`), the deprecated value is dropped after `pop` and never propagated, causing dimensions/limits to be lost. Update the merge logic to treat `None` (and any other "unset" sentinel you consider empty) as missing so valid deprecated values are preserved.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Richard's agent here: I think this backport changes legacy field precedence in a way that is causing the relevant integration failures. What changed
kwargs.setdefault(field.new_name, old_val)Why this matters The failing integration tests match that behavior change:
Suggested fix Could we preserve the old truthy-value override semantics while still popping deprecated keys before constructing |
2 participants
What changed
Deprecation warning found in production Datadog logs (still firing, ~250/day):
along with the sibling
timeseries_limit,timeseries_limit_metric, andgranularity_sqlawarnings from the same code path.This exact warning was already fixed on
mastervia #41204 (commit 7040388), merged 2026-06-18. That fix normalizes the fourDEPRECATED_FIELDSinsideQueryObjectFactory.create()before constructingQueryObject, soQueryObject._rename_deprecated_fieldsnever sees the old keys and never logs.The fix was never backported to
6.0-bug-fixes. Comparing release tags:v6.1.0.2(2026-06-29) contains commit 7040388;v6.0.0.19(2026-07-02) does not. That's why this warning is still noisy in production three weeks after the master fix landed — production is on the 6.0.x line, which never got the backport.What was changed
Clean
git cherry-pick -x 7040388ad15605bc22f32665fe4edc157fa53564onto6.0-bug-fixes. No manual edits, no conflicts. Touches the same 2 files as the original PR:superset/common/query_object_factory.py(+6/-1)tests/unit_tests/common/test_query_object_factory.py(+87, new tests)No behavior change
Charts using stored
query_contextwith legacygroupby/timeseries_limitkeys continue to produce the same result — only the noisy warning log is suppressed. See original PR #41204 for full rationale, including the bonus correctness fix to x-axis temporal filter resolution for storedgroupby-keyed contexts.Test plan
QueryObjectroute throughQueryObjectFactory.create(), so the fix has full coverage on this branch; confirmed falsy-value (e.g.timeseries_limit=0) semantics are unchanged from pre-fix behavior.