korbit-ai[bot] commented on code in PR #35937:
URL: https://github.com/apache/superset/pull/35937#discussion_r2482910852
##########
superset/models/helpers.py:
##########
@@ -1342,6 +1342,53 @@ def _get_series_orderby(
)
return ob
+ def _reapply_query_filters(
+ self,
+ qry: Select,
+ apply_fetch_values_predicate: bool,
+ template_processor: Optional[BaseTemplateProcessor],
+ granularity: str | None,
+ time_filters: list[ColumnElement],
+ where_clause_and: list[ColumnElement],
+ having_clause_and: list[ColumnElement],
+ ) -> Select:
Review Comment:
### Too Many Parameters in Method <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The method has too many parameters (7) which makes it harder to maintain and
use correctly.
###### Why this matters
Having too many parameters increases cognitive load, makes the method harder
to test, and increases the likelihood of errors when calling the method.
###### Suggested change ∙ *Feature Preview*
Create a dedicated class or dataclass to encapsulate the filter parameters:
```python
@dataclass
class QueryFilters:
apply_fetch_values_predicate: bool
template_processor: Optional[BaseTemplateProcessor]
granularity: str | None
time_filters: list[ColumnElement]
where_clause_and: list[ColumnElement]
having_clause_and: list[ColumnElement]
def _reapply_query_filters(self, qry: Select, filters: QueryFilters) ->
Select:
# Method implementation
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d5fd715-da4e-4900-87a3-ca64fe6dee54/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d5fd715-da4e-4900-87a3-ca64fe6dee54?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d5fd715-da4e-4900-87a3-ca64fe6dee54?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d5fd715-da4e-4900-87a3-ca64fe6dee54?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5d5fd715-da4e-4900-87a3-ca64fe6dee54)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6f099583-53f8-426b-985d-f6848ca38b86 -->
[](6f099583-53f8-426b-985d-f6848ca38b86)
##########
superset/models/helpers.py:
##########
@@ -1342,6 +1342,53 @@ def _get_series_orderby(
)
return ob
+ def _reapply_query_filters(
+ self,
+ qry: Select,
+ apply_fetch_values_predicate: bool,
+ template_processor: Optional[BaseTemplateProcessor],
+ granularity: str | None,
+ time_filters: list[ColumnElement],
+ where_clause_and: list[ColumnElement],
+ having_clause_and: list[ColumnElement],
+ ) -> Select:
+ """
+ Re-apply WHERE and HAVING clauses to a reconstructed query.
+
+ When group_others_when_limit_reached=True, the query is reconstructed
+ with sa.select(), losing previously applied filters. This method
+ re-applies those filters to maintain query correctness.
+
+ The WHERE clause includes: user filters, RLS filters, extra WHERE
+ clauses, and time range filters accumulated in where_clause_and
+ and time_filters.
+
+ :param qry: The reconstructed SQLAlchemy Select object
+ :param apply_fetch_values_predicate: Whether to apply fetch values
predicate
+ :param template_processor: Template processor for dynamic filters
+ :param granularity: Time granularity (if None, time_filters not
applied)
+ :param time_filters: Time-based filter conditions
+ :param where_clause_and: Accumulated WHERE clause conditions
+ :param having_clause_and: Accumulated HAVING clause conditions
+ :return: The query with filters re-applied
+ """
+ if apply_fetch_values_predicate and self.fetch_values_predicate:
+ qry = qry.where(
+
self.get_fetch_values_predicate(template_processor=template_processor)
+ )
+
+ if granularity:
+ if time_filters or where_clause_and:
+ qry = qry.where(and_(*(time_filters + where_clause_and)))
+ else:
+ if where_clause_and:
+ qry = qry.where(and_(*where_clause_and))
Review Comment:
### Time filters ignored when granularity is falsy <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logic for applying time_filters is inconsistent - time_filters are only
applied when granularity is truthy, but time_filters can exist even when
granularity is None or falsy.
###### Why this matters
This could result in time-based filters being silently ignored when
granularity is None, leading to incorrect query results that include data
outside the intended time range.
###### Suggested change ∙ *Feature Preview*
Apply time_filters regardless of granularity status:
```python
if granularity:
if time_filters or where_clause_and:
qry = qry.where(and_(*(time_filters + where_clause_and)))
else:
# Apply both time_filters and where_clause_and even without granularity
all_filters = time_filters + where_clause_and
if all_filters:
qry = qry.where(and_(*all_filters))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5227939-4049-41b5-a808-2a341f7f513e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5227939-4049-41b5-a808-2a341f7f513e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5227939-4049-41b5-a808-2a341f7f513e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5227939-4049-41b5-a808-2a341f7f513e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5227939-4049-41b5-a808-2a341f7f513e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d6973b43-0a14-4f26-bbd6-359c43cea5ed -->
[](d6973b43-0a14-4f26-bbd6-359c43cea5ed)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
