codeant-ai-for-open-source[bot] commented on code in PR #41556:
URL: https://github.com/apache/superset/pull/41556#discussion_r3530725522
##########
superset/semantic_layers/mapper.py:
##########
@@ -390,12 +390,22 @@ def _get_filters_from_query_object(
extras_filters = _get_filters_from_extras(query_object.extras)
filters.update(extras_filters)
- # 4. Add all other filters from query_object.filter
+ # 4. Add all other filters from query_object.filter.
+ # ``TEMPORAL_RANGE`` filters are skipped only when ``_get_time_filter``
+ # actually emitted bounds — that path takes over both the base range and
+ # the ``time_offset`` shift, so pass-through would duplicate the bounds
+ # (or, worse, ship the un-shifted literal bounds into the offset query).
+ # When it did not emit anything (e.g. an open-ended range like
+ # ``"2020-01-01 : "``, where ``_get_time_filter`` requires both
+ # ``from_dttm`` and ``to_dttm``), we fall through to
+ # ``_convert_query_object_filter``'s TEMPORAL_RANGE handler so the one-
+ # sided predicate still lands on the query instead of silently widening
+ # the scan.
+ time_bounds_emitted = bool(time_filters)
for filter_ in query_object.filter:
- # Skip temporal range filters - we're using inner bounds instead
if (
filter_.get("op") == FilterOperator.TEMPORAL_RANGE.value
- and query_object.granularity
+ and time_bounds_emitted
):
continue
Review Comment:
**Suggestion:** The new TEMPORAL_RANGE skip condition drops every
temporal-range adhoc filter whenever any time bounds were emitted, even if the
filter targets a different temporal column than the chosen time axis. In
queries that contain multiple temporal-range filters, this silently removes
non-axis constraints and broadens the result set. Restrict the skip to only
TEMPORAL_RANGE filters whose `col` matches the resolved time-axis column.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Semantic-layer queries ignore non-axis temporal-range constraints.
- ⚠️ Dashboards with extra date filters show unexpectedly broadened data.
- ⚠️ Time-comparison charts return misleading metrics for filtered periods.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a `ValidatedQueryObject` as in
`tests/unit_tests/semantic_layers/mapper_test.py:test_get_filters_from_query_object_with_filter_clauses`
(`mapper_test.py:4-44`), but extend `filter` to contain **two**
`TEMPORAL_RANGE` clauses:
one on the main time-axis column (e.g. `"order_date"`) and another on a
different temporal
column (e.g. `"ship_date"`), both allowed by
`ValidatedQueryObjectFilterClause` in
`superset/semantic_layers/mapper.py:13-24`.
2. Ensure `all_dimensions` includes both temporal dimensions so
`_get_time_axis_column()`
in `superset/semantic_layers/mapper.py:33-79` resolves the time axis (e.g.
`"order_date"`
via `query_object.granularity`) and `_get_time_filter()` in
`mapper.py:82-116` emits
`time_filters` for that axis using `from_dttm`/`to_dttm`.
3. Call `map_query_object(query_object)` (entry at
`superset/semantic_layers/mapper.py:61-65`), which invokes
`_get_filters_from_query_object(query_object, time_offset, all_dimensions)`
at
`mapper.py:18-22`; inside it, `time_filters` is computed and
`time_bounds_emitted =
bool(time_filters)` is set at `mapper.py:404`.
4. In `_get_filters_from_query_object`, the loop at `mapper.py:405-413`
evaluates each
filter: for both `TEMPORAL_RANGE` clauses (including the one on
`"ship_date"`), the
condition `filter_.get("op") == FilterOperator.TEMPORAL_RANGE.value and
time_bounds_emitted` (lines 406-410) is true, so `continue` is executed and
`_convert_query_object_filter()` (lines 414-417) is never called, resulting
in **all**
adhoc temporal-range filters being dropped; the final `filters` set contains
only the axis
time filters from `_get_time_filter` and any extras, silently removing the
secondary
temporal constraint and broadening the result set.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0bddabb59f274acdb5138182d2e32921&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=0bddabb59f274acdb5138182d2e32921&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:** superset/semantic_layers/mapper.py
**Line:** 406:410
**Comment:**
*Logic Error: The new TEMPORAL_RANGE skip condition drops every
temporal-range adhoc filter whenever any time bounds were emitted, even if the
filter targets a different temporal column than the chosen time axis. In
queries that contain multiple temporal-range filters, this silently removes
non-axis constraints and broadens the result set. Restrict the skip to only
TEMPORAL_RANGE filters whose `col` matches the resolved time-axis column.
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%2F41556&comment_hash=df2449844cc2641ad6ae50454b33bc6020a2fce2a05f137af91eaff7de8415f0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41556&comment_hash=df2449844cc2641ad6ae50454b33bc6020a2fce2a05f137af91eaff7de8415f0&reaction=dislike'>👎</a>
##########
superset/semantic_layers/mapper.py:
##########
@@ -889,12 +954,14 @@ def _get_group_limit_filters(
extras_filters = _get_filters_from_extras(query_object.extras)
filters.update(extras_filters)
- # Add all other non-temporal filters from query_object.filter
+ # Add all other non-temporal filters from query_object.filter. Skip
+ # ``TEMPORAL_RANGE`` only when the inner-bound filters were actually
+ # emitted — otherwise dropping the pass-through would silently widen the
+ # group-limit subquery to the full history.
for filter_ in query_object.filter:
- # Skip temporal range filters - we're using inner bounds instead
if (
filter_.get("op") == FilterOperator.TEMPORAL_RANGE.value
- and query_object.granularity
+ and time_bounds_emitted
):
continue
Review Comment:
**Suggestion:** The same broad TEMPORAL_RANGE skip was added to group-limit
filter construction, so when inner bounds are emitted the code also removes
other temporal-range adhoc filters that should still apply to the subquery.
This can produce incorrect top-N groups by widening group-limit scope. Skip
only the temporal-range filter bound to the resolved time-axis column, not all
temporal-range filters. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Group-limit subqueries ignore extra temporal-range filters.
- ⚠️ Top-N results on semantic charts can be significantly wrong.
- ⚠️ Dashboards using time comparison and limits see skewed rankings.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the `mock_datasource` fixture from
`tests/unit_tests/semantic_layers/mapper_test.py:1-62` and adapt
`test_get_group_limit_filters_different_bounds` (`mapper_test.py:110-131`)
to add a
`filter` list on the `ValidatedQueryObject` containing a `TEMPORAL_RANGE`
filter for the
time-axis column (e.g. `"order_date"`) plus an additional `TEMPORAL_RANGE`
filter on
another temporal column (e.g. `"ship_date"`).
2. Configure `from_dttm`/`to_dttm` and `inner_from_dttm`/`inner_to_dttm` so
that inner
bounds differ from outer bounds, as in
`test_get_group_limit_filters_different_bounds`
(`mapper_test.py:118-127`), ensuring `_get_group_limit_filters()` is used via
`_get_group_limit_from_query_object()` in
`superset/semantic_layers/mapper.py:13-35`.
3. When `_get_group_limit_filters()` runs (`mapper.py:46-58`), it resolves
the time-axis
column via `_get_time_axis_column()` (`mapper.py:75-80`), emits inner-bound
time filters
for that column and sets `time_bounds_emitted = True` at `mapper.py:71-79,
19`, then adds
any extras/fetch predicates (`mapper.py:21-37`).
4. The subsequent loop over `query_object.filter` at
`superset/semantic_layers/mapper.py:39-52` applies the condition
`filter_.get("op") ==
FilterOperator.TEMPORAL_RANGE.value and time_bounds_emitted` (lines 962-966)
to **every**
`TEMPORAL_RANGE` filter; both the axis and non-axis temporal-range filters
are skipped via
`continue`, so `_convert_query_object_filter()` is never invoked for the
additional
temporal constraint, and the `GroupLimit.filters` attached in
`_get_group_limit_from_query_object()` (lines 32-42) omit that filter,
causing the
group-limit subquery to run over a wider time window than intended and
potentially
selecting incorrect top-N groups.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e10c60ff41494185bfcc8910d605c007&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=e10c60ff41494185bfcc8910d605c007&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:** superset/semantic_layers/mapper.py
**Line:** 962:966
**Comment:**
*Logic Error: The same broad TEMPORAL_RANGE skip was added to
group-limit filter construction, so when inner bounds are emitted the code also
removes other temporal-range adhoc filters that should still apply to the
subquery. This can produce incorrect top-N groups by widening group-limit
scope. Skip only the temporal-range filter bound to the resolved time-axis
column, not all temporal-range filters.
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%2F41556&comment_hash=87cad5071d8884b77323578e89cde3ba02d554e2f8d2dab3466c7afadba75405&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41556&comment_hash=87cad5071d8884b77323578e89cde3ba02d554e2f8d2dab3466c7afadba75405&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: [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]
