fix(explore): re-apply filters when 'Group remaining as Others' is enabled #35937

richardfogaca · 2025-10-31T23:06:15Z

SUMMARY

Fixes a bug where charts with "Group remaining as Others" enabled ignore all filters (time range, user filters, RLS filters, custom WHERE clauses).

Problem

When group_others_when_limit_reached=True, the query is reconstructed with sa.select(select_exprs), creating a new SQLAlchemy Select object that loses previously applied WHERE and HAVING clauses.

Solution

Added _reapply_query_filters() method to systematically re-apply all filters after query reconstruction
Fixed time_filters initialization - moved before if granularity: block to prevent undefined variable error when charts have no time dimension
Called filter reapplication at both query reconstruction points (lines 2449 and 2523)

Changes

superset/models/helpers.py: New method + initialization fix + 2 integration points
tests/unit_tests/models/helpers_test.py: 5 unit tests covering all code paths

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE: Outer WHERE clause missing when "Group remaining as Others" is enabled

AFTER: Outer WHERE clause correctly applied

TESTING INSTRUCTIONS

Manual Testing

Create a bar chart with:
- Time dimension + categorical series column
- Set time range filter (e.g., "Last 30 days")
- Enable series "LIMIT" (e.g., 10) and check "Group remaining as 'Others'"
Verify:
- Chart shows only data within selected time range
- SQL query has outer WHERE clause with time filters
- "Others" category appears for remaining series
Test edge case: Create chart without time dimension, add filters, enable "Group remaining as Others" - should work without errors

ADDITIONAL INFORMATION

Fixes #35815

Has associated issue: [6.0.0-RC2] >>Group remaining as "Others"<< generates an incorrect SQL Query #35815
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…abled

bito-code-review · 2025-10-31T23:06:24Z

Code Review Agent Run #b09b3a

Actionable Suggestions - 0

Additional Suggestions - 8

superset/models/helpers.py - 5

Boolean positional argument in function · Line 1348-1348

Boolean parameter `apply_fetch_values_predicate` should be keyword-only to improve readability and prevent accidental misuse.

Code suggestion

 @@ -1345,3 +1345,3 @@
-    def _reapply_query_filters(
-        self,
-        qry: Select,
-        apply_fetch_values_predicate: bool,
+    def _reapply_query_filters(
+        self,
+        qry: Select,
+        *,
+        apply_fetch_values_predicate: bool,

Use union operator for type annotations · Line 1349-1349

Type annotation `Optional[BaseTemplateProcessor]` should use the modern union syntax `BaseTemplateProcessor | None` for Python 3.10+ compatibility.
Code suggestion
```
 @@ -1349,1 +1349,1 @@
-        template_processor: Optional[BaseTemplateProcessor],
+        template_processor: BaseTemplateProcessor | None,
```

Multi-line docstring format issue · Line 1355-1355

Multi-line docstring should start the summary on the first line instead of having it on a separate line.

Code suggestion

 @@ -1354,3 +1354,2 @@
-    ) -> Select:
-        """
-        Re-apply WHERE and HAVING clauses to a reconstructed query.
+    ) -> Select:
+        """Re-apply WHERE and HAVING clauses to a reconstructed query.

Missing trailing comma in function call · Line 1377-1377

Missing trailing comma after `template_processor=template_processor`. There are 2 other similar trailing comma issues at lines 2029 and 2455.

Code suggestion

 @@ -1377,1 +1377,1 @@
-                self.get_fetch_values_predicate(template_processor=template_processor)
+                self.get_fetch_values_predicate(template_processor=template_processor,)

Use elif instead of else if · Line 1383-1383

Use `elif` instead of `else` followed by `if` to reduce indentation level and improve readability.
Code suggestion
```
 @@ -1383,2 +1383,1 @@
-        else:
-            if where_clause_and:
+        elif where_clause_and:
```

tests/unit_tests/models/helpers_test.py - 3

Multi-line docstring summary formatting issue · Line 1132-1132

Multi-line docstring summary should start at the first line. Multiple similar issues exist throughout the file (lines 1187, 1239, 1289, 1344).

Code suggestion

 @@ -1131,6 +1131,5 @@
 
  def test_reapply_query_filters_with_granularity(database: Database) -> None:
-    """
-    Test that _reapply_query_filters correctly applies filters with granularity.
+    """Test that _reapply_query_filters correctly applies filters with granularity.
 
      When granularity is provided, both time_filters and where_clause_and should

Magic value used in comparison · Line 1154-1154

Replace magic value `10` with a named constant for better maintainability. Multiple similar issues exist with values `100` (line 1261).

Code suggestion

 @@ -1151,6 +1151,8 @@
      # Create mock filter conditions
      time_filter = sa.column("time_col") >= "2025-01-01"
-    where_filter = sa.column("value") > 10
+    MIN_VALUE_THRESHOLD = 10
+    where_filter = sa.column("value") > MIN_VALUE_THRESHOLD

Missing trailing comma in function call · Line 1175-1175

Add trailing comma after `compile_kwargs={"literal_binds": True}` for consistency. Multiple similar issues exist throughout the file (lines 1176, 1229, 1230, 1257, 1279, 1280, 1314, 1334, 1335, 1385, 1386).

Code suggestion

 @@ -1173,7 +1173,7 @@
          sql = str(
              result_qry.compile(
-                dialect=engine.dialect, compile_kwargs={"literal_binds": True}
+                dialect=engine.dialect, compile_kwargs={"literal_binds": True},
              )
          )

Review Details

Files reviewed - 2 · Commit Range: a08b739..a08b739
- superset/models/helpers.py
- tests/unit_tests/models/helpers_test.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

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 documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Too Many Parameters in Method ▹ view
	Time filters ignored when granularity is falsy ▹ view	✅ Fix detected

Files scanned

File Path	Reviewed
superset/models/helpers.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

superset/models/helpers.py

codecov · 2025-10-31T23:14:36Z

Codecov Report

❌ Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.72%. Comparing base (6e27bee) to head (9d222c4).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/models/helpers.py	13.33%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #35937       +/-   ##
===========================================
+ Coverage        0   68.72%   +68.72%     
===========================================
  Files           0      622      +622     
  Lines           0    45726    +45726     
  Branches        0     4977     +4977     
===========================================
+ Hits            0    31423    +31423     
- Misses          0    13058    +13058     
- Partials        0     1245     +1245

Flag	Coverage Δ
hive	`44.20% <6.66%> (?)`
mysql	`67.79% <13.33%> (?)`
postgres	`67.84% <13.33%> (?)`
presto	`47.74% <13.33%> (?)`
python	`68.68% <13.33%> (?)`
sqlite	`67.46% <13.33%> (?)`
unit	`100.00% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

fix(explore): re-apply filters when 'Group remaining as Others' is en…

a08b739

…abled

pull-request-size bot added the size/L label Oct 31, 2025

dosubot bot added the change:backend Requires changing the backend label Oct 31, 2025

richardfogaca mentioned this pull request Oct 31, 2025

[6.0.0-RC2] >>Group remaining as "Others"<< generates an incorrect SQL Query #35815

Open

3 tasks

korbit-ai bot suggested changes Oct 31, 2025

View reviewed changes

superset/models/helpers.py Outdated Show resolved Hide resolved

superset/models/helpers.py Show resolved Hide resolved

refactor: apply time_filters regardless of granularity

9d222c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(explore): re-apply filters when 'Group remaining as Others' is enabled #35937

fix(explore): re-apply filters when 'Group remaining as Others' is enabled #35937

richardfogaca commented Oct 31, 2025 •

edited

Loading

bito-code-review bot commented Oct 31, 2025 •

edited

Loading

Code Review Agent Run #b09b3a

korbit-ai bot left a comment •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented Oct 31, 2025 •

edited

Loading

Labels

1 participant

fix(explore): re-apply filters when 'Group remaining as Others' is enabled #35937

Are you sure you want to change the base?

fix(explore): re-apply filters when 'Group remaining as Others' is enabled #35937

Conversation

richardfogaca commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

Problem

Solution

Changes

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Manual Testing

ADDITIONAL INFORMATION

bito-code-review bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #b09b3a

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

Uh oh!

codecov bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Labels

1 participant

richardfogaca commented Oct 31, 2025 •

edited

Loading

bito-code-review bot commented Oct 31, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading

codecov bot commented Oct 31, 2025 •

edited

Loading