You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/07/11 18:13:29 UTC
[PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
rusackas opened a new pull request, #41960:
URL: https://github.com/apache/superset/pull/41960
### SUMMARY
Test-only regression coverage for #28562, where **"Drill to Detail by"** launched from a chart on a dashboard reportedly ignores the dashboard's applied filters β the drill-to-detail samples query returns rows that are not scoped to the dashboard's filter selections.
This PR adds a focused unit test against the backend drill-to-detail action, `superset.common.query_actions._get_drill_detail`. That function is the samples/drill-detail code path: it rewrites the query object's columns, metrics, post-processing and orderby before executing the samples query. The test asserts that the transformation leaves `QueryObject.filter` (the chart's applied filters) untouched, so the filters survive onto the query object that is ultimately executed.
No production code is changed. This is a diagnostic/characterization test to pin down whether the reported behavior reproduces in the backend filter-merge path.
### How to interpret CI
- **Green** (current result locally): the backend drill-detail path *preserves* the applied filters. The reported bug is **not** reproduced in this code path, which points the investigation toward the frontend payload assembly (`getDrillPayload` / dashboard native-filter propagation into the chart form data) rather than the Python samples path.
- **Red**: the drill-detail transformation drops the applied filters before execution β a live backend reproduction of #28562.
Either way the test is a permanent regression guard: it will fail if a future change causes the drill-detail action to strip filters.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/common/test_query_actions_currency.py::test_get_drill_detail_preserves_applied_filters -v
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue: Closes #28562
- [x] Required feature flags: n/a
- [x] Changes UI: no
- [x] Includes DB Migration: no
- [x] Introduces new feature or API: no
- [x] Removes existing feature or API: no
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41960:
URL: https://github.com/apache/superset/pull/41960
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4948290882
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41960?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 63.97%. Comparing base ([`df209ce`](https://app.codecov.io/gh/apache/superset/commit/df209cedbf851ba7f96e8deff0e94446504e6d1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`262c63e`](https://app.codecov.io/gh/apache/superset/commit/262c63e5c2f4ab5a806c993a6f04ac2af626ce12?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41960 +/- ##
==========================================
- Coverage 64.77% 63.97% -0.81%
==========================================
Files 2742 2742
Lines 153316 153316
Branches 35143 35143
==========================================
- Hits 99311 98083 -1228
- Misses 52107 53342 +1235
+ Partials 1898 1891 -7
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.07% <ΓΈ> (ΓΈ)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.06% <ΓΈ> (ΓΈ)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.79% <ΓΈ> (-1.64%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.65% <ΓΈ> (ΓΈ)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41960?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4974615375
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4972815076
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3564771885
##########
tests/unit_tests/common/test_query_actions_currency.py:
##########
@@ -288,3 +290,60 @@ def test_detect_currency_column_config_no_currency_column_returns_none(
)
assert result is None
+
+
+# Tests for drill-to-detail filter propagation (issue #28562)
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_preserves_applied_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Regression for #28562: 'Drill to Detail by' must carry a chart's applied
+ filters (e.g. the dashboard's native filters) into the drill-to-detail
+ samples query. The drill-detail action rewrites columns/metrics/orderby but
+ must leave ``QueryObject.filter`` untouched so the underlying rows stay
+ scoped to the dashboard's filter selections.
+
+ This exercises the backend drill-detail transformation directly and asserts
+ the incoming filters survive onto the query object that is ultimately
+ executed. A green run means the backend honors the filters (the reported
+ behavior is not reproduced in this code path); a red run means they are
+ dropped.
+ """
+ applied_filter = {"col": "region", "op": "==", "val": "USA"}
Review Comment:
**Suggestion:** Add an explicit type annotation for this local filter payload variable to satisfy the requirement for annotating relevant new variables. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The new local variable is a plain dictionary literal with no type annotation, and the rule explicitly requires type hints on relevant variables that can be annotated. This is a real instance of the stated violation.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=871162817335444998a6703febdf2113&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=871162817335444998a6703febdf2113&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_actions_currency.py
**Line:** 315:315
**Comment:**
*Custom Rule: Add an explicit type annotation for this local filter payload variable to satisfy the requirement for annotating relevant new variables.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=7b7f612ef3c82c9e3ed880f51dc20c317277cbe924f976a32bbde3103667024b&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=7b7f612ef3c82c9e3ed880f51dc20c317277cbe924f976a32bbde3103667024b&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3578560022
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation to this local variable so the constructed query object is typed. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a new local variable whose type is clearly inferable as QueryObject, so it falls under the rule requiring type hints for relevant annotatable variables.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cffea2cc67b946a7b335f708f15e79b9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cffea2cc67b946a7b335f708f15e79b9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_actions.py
**Line:** 56:60
**Comment:**
*Custom Rule: Add an explicit type annotation to this local variable so the constructed query object is 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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=35c5944dacec29436a037dcba363b95152e8f1d16de4705a4a9b44e8f4593c32&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=35c5944dacec29436a037dcba363b95152e8f1d16de4705a4a9b44e8f4593c32&reaction=dislike'>π</a>
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
+ col_region.column_name = "region"
+ col_sales = MagicMock()
+ col_sales.column_name = "sales"
+
+ datasource = MagicMock()
+ datasource.columns = [col_region, col_sales]
+
+ query_context = MagicMock()
Review Comment:
**Suggestion:** Add a type hint for this local variable to keep the test context object explicitly typed. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a newly added local variable and its intended role is explicit in the test, so it can be annotated; omitting a type hint matches the custom rule violation.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8d38f11d817145acadd762b6c4d460cd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8d38f11d817145acadd762b6c4d460cd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_actions.py
**Line:** 70:70
**Comment:**
*Custom Rule: Add a type hint for this local variable to keep the test context object explicitly 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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=fd815bd5db8f0b545d34c6fbf6df2989f57875c4211c482e7b15ed0fa42901f7&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=fd815bd5db8f0b545d34c6fbf6df2989f57875c4211c482e7b15ed0fa42901f7&reaction=dislike'>π</a>
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
+ col_region.column_name = "region"
+ col_sales = MagicMock()
+ col_sales.column_name = "sales"
+
+ datasource = MagicMock()
+ datasource.columns = [col_region, col_sales]
+
+ query_context = MagicMock()
+ query_context.datasource = datasource
+ query_context.result_type = ChartDataResultType.DRILL_DETAIL
+
+ captured: dict[str, QueryObject] = {}
+
+ def _capture(_ctx: MagicMock, obj: QueryObject, _force: bool) -> dict[str, Any]:
+ captured["query_obj"] = obj
+ return {}
+
+ mock_get_full.side_effect = _capture
+
+ _get_drill_detail(query_context, query_obj)
+
+ executed = captured["query_obj"]
Review Comment:
**Suggestion:** Add a concrete type annotation for this extracted result variable to satisfy the type-hint requirement. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a new local variable whose value is drawn from a typed container, and it can be annotated as QueryObject; the omission of a type hint is a real instance of the rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9604fc0a50a64b219ed0d663ed4ead60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9604fc0a50a64b219ed0d663ed4ead60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_actions.py
**Line:** 84:84
**Comment:**
*Custom Rule: Add a concrete type annotation for this extracted result variable to satisfy the type-hint requirement.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=8a9c4940c5a9265d7e4d811e05db05cce0c114918966ed527711a98f15aefb36&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=8a9c4940c5a9265d7e4d811e05db05cce0c114918966ed527711a98f15aefb36&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4968658981
@sadpandajoe @rebenitez1802 good catch, both. `_get_drill_detail` never reads or reassigns `query_obj.filter`, so this test couldn't have caught #28562 regardless of how it's written. Moved it to its own `test_query_actions.py` and reframed the docstring/name so it's explicit this is a characterization test of that pass-through behavior, not a reproduction of the reported bug. The actual root cause is still unconfirmed and likely upstream in the payload assembly, not in this function.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4960374880
π‘ Medium β Test canβt catch the bug itβs named for
test_get_drill_detail_preserves_applied_filters asserts filters survive _get_drill_detail, but that function (superset/common/query_actions.py:227) never reads or mutates query_obj.filter β it shallow-copies (copy.copy) and rewrites only is_timeseries, metrics, post_processing, columns, orderby. Since the copy shares the same filter list object, the assertion is effectively guaranteed to pass and can only fail if someone deliberately adds filter-stripping code here. The PR body concedes green β bug reproduced, so this guards a path that was never the source of #28562 (which lives upstream in native-filter propagation / getDrillPayload / the query-context filter merge). Fix: retarget the test at the actual filter-assembly path, or rename/reframe it explicitly as a pass-through characterization test so it isnβt mistaken for a real regression guard.
π’ Low β Wrong file
Added to test_query_actions_currency.py, a currency-detection-specific file; drill-detail tests donβt belong there. Fix: move to a new test_query_actions.py (none exists yet) for discoverability.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3580503121
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj: QueryObject = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
Review Comment:
**Suggestion:** Add an explicit type annotation for this mock column variable to satisfy the requirement for annotating relevant variables. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The variable `col_region` is a newly added Python local variable and is not type annotated, while the rule requires type hints on relevant variables that can be annotated. This is a real violation in the shown code.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b2d991c08a8a4980bbc372b5e7df9c5d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b2d991c08a8a4980bbc372b5e7df9c5d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_query_actions.py
**Line:** 62:62
**Comment:**
*Custom Rule: Add an explicit type annotation for this mock column variable to satisfy the requirement for annotating relevant variables.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=1b56f4ff17957408335428fe020781b25a68e49bc6716014a687e577a1aab8fe&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41960&comment_hash=1b56f4ff17957408335428fe020781b25a68e49bc6716014a687e577a1aab8fe&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4948887730
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3564824972
##########
tests/unit_tests/common/test_query_actions_currency.py:
##########
@@ -288,3 +290,60 @@ def test_detect_currency_column_config_no_currency_column_returns_none(
)
assert result is None
+
+
+# Tests for drill-to-detail filter propagation (issue #28562)
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_preserves_applied_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Regression for #28562: 'Drill to Detail by' must carry a chart's applied
+ filters (e.g. the dashboard's native filters) into the drill-to-detail
+ samples query. The drill-detail action rewrites columns/metrics/orderby but
+ must leave ``QueryObject.filter`` untouched so the underlying rows stay
+ scoped to the dashboard's filter selections.
+
+ This exercises the backend drill-detail transformation directly and asserts
+ the incoming filters survive onto the query object that is ultimately
+ executed. A green run means the backend honors the filters (the reported
+ behavior is not reproduced in this code path); a red run means they are
+ dropped.
+ """
+ applied_filter = {"col": "region", "op": "==", "val": "USA"}
Review Comment:
Good call, typed the filter payload as QueryObjectFilterClause and parametrized the capture return dict so mypy's happy now.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4968704890
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | e2bc6564b107fe3e713896a71405f96f5d75ae85 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a561dbd483c990008bd6083 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-41960--superset-docs-preview.netlify.app](https://deploy-preview-41960--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctNDoMgEAbQu8zaidQfgr1BjwF-NCUZC4FxYUzvXnZv824qNR9F6Uk0UKj-u3-6FRhTa2fkya12YtQkwpoZUX0SfifRWFsviEXy9UJP1q_2gYDFzfu2GWNcgDVupt8f) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41960--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3581995958
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj: QueryObject = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
Review Comment:
Good catch, annotated `col_region` and `col_sales` as `MagicMock` to match the rest of the file.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41960:
URL: https://github.com/apache/superset/pull/41960#discussion_r3580492789
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
Review Comment:
Added an explicit `QueryObject` annotation on `query_obj`.
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
+ col_region.column_name = "region"
+ col_sales = MagicMock()
+ col_sales.column_name = "sales"
+
+ datasource = MagicMock()
+ datasource.columns = [col_region, col_sales]
+
+ query_context = MagicMock()
Review Comment:
Added an explicit `MagicMock` annotation on `query_context`.
##########
tests/unit_tests/common/test_query_actions.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+from superset.common.chart_data import ChartDataResultType
+from superset.common.query_actions import _get_drill_detail
+from superset.common.query_object import QueryObject
+from superset.utils.core import QueryObjectFilterClause
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_drill_detail_does_not_strip_filters(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ Characterization test for ``_get_drill_detail`` (superset/common/query_actions.py),
+ the backend transform behind the "Drill to Detail by" samples query.
+
+ ``_get_drill_detail`` shallow-copies the incoming ``QueryObject`` and rewrites
+ ``is_timeseries``, ``metrics``, ``post_processing``, ``columns`` and ``orderby``;
+ it never reads or reassigns ``QueryObject.filter``. This test pins that
+ behavior down: filters present on the ``QueryObject`` handed to
+ ``_get_drill_detail`` are still present, unmodified, on the object it hands off
+ to ``_get_full``.
+
+ This is deliberately narrow and does NOT reproduce or close #28562 ("Drill to
+ Detail by" ignoring a dashboard's applied filters) -- that report describes
+ filters missing from the query *before* it reaches this function, which points
+ at the payload assembled upstream of ``_get_drill_detail`` (dashboard native
+ filter propagation into the chart's form data / the drill payload built on the
+ frontend), not at this transform. If that assembly is ever changed to drop
+ filters before calling into this code path, this test would not catch it; it
+ only guards against a regression introduced inside ``_get_drill_detail`` itself.
+ """
+ applied_filter: QueryObjectFilterClause = {
+ "col": "region",
+ "op": "==",
+ "val": "USA",
+ }
+
+ query_obj = QueryObject(
+ columns=["region", "sales"],
+ metrics=["count"],
+ filters=[applied_filter],
+ )
+
+ col_region = MagicMock()
+ col_region.column_name = "region"
+ col_sales = MagicMock()
+ col_sales.column_name = "sales"
+
+ datasource = MagicMock()
+ datasource.columns = [col_region, col_sales]
+
+ query_context = MagicMock()
+ query_context.datasource = datasource
+ query_context.result_type = ChartDataResultType.DRILL_DETAIL
+
+ captured: dict[str, QueryObject] = {}
+
+ def _capture(_ctx: MagicMock, obj: QueryObject, _force: bool) -> dict[str, Any]:
+ captured["query_obj"] = obj
+ return {}
+
+ mock_get_full.side_effect = _capture
+
+ _get_drill_detail(query_context, query_obj)
+
+ executed = captured["query_obj"]
Review Comment:
Added an explicit `QueryObject` annotation on `executed`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4970365404
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4948243330
<!-- Bito Reply -->
The flagged issue is correct. The variable `applied_filter` is defined as a dictionary literal without an explicit type annotation, which violates the project's standard for annotating new variables.
To resolve this, you can add the type annotation `dict[str, str]` to the variable definition. Here is the corrected code snippet:
```python
applied_filter: dict[str, str] = {"col": "region", "op": "==", "val": "USA"}
query_obj = QueryObject(
```
There are no other comments on this pull request to address. Would you like me to proceed with any other tasks?
**tests/unit_tests/common/test_query_actions_currency.py**
```
applied_filter: dict[str, str] = {"col": "region", "op": "==", "val": "USA"}
query_obj = 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(charts): assert drill-to-detail carries applied filters (#28562) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41960:
URL: https://github.com/apache/superset/pull/41960#issuecomment-4948238281
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org