codeant-ai-for-open-source[bot] commented on code in PR #41339:
URL: https://github.com/apache/superset/pull/41339#discussion_r4341277152
##########
superset/common/query_context_factory.py:
##########
@@ -296,25 +296,41 @@ def _apply_granularity( # noqa: C901
if (column["is_dttm"] if isinstance(column, dict) else
column.is_dttm)
}
x_axis = form_data and form_data.get("x_axis")
- temporal_range_filters = [
+ main_dttm_col = getattr(datasource, "main_dttm_col", None)
+ # Filters that would be clobbered by inferring main_dttm_col below: a
+ # TEMPORAL_RANGE filter already on main_dttm_col itself isn't
+ # "independent", it's the same time subject inference would also
+ # pick, and gets deduped once inference runs.
+ other_temporal_filters = [
filter_
for filter_ in query_object.filter
- if filter_["op"] == "TEMPORAL_RANGE"
+ if filter_["op"] == "TEMPORAL_RANGE" and filter_["col"] !=
main_dttm_col
]
should_infer_filter_granularity: bool = (
is_adhoc_column(x_axis) # type: ignore
and query_object.granularity is None
and bool(query_object.from_dttm or query_object.to_dttm)
- and (bool(query_object.time_range) or not temporal_range_filters)
- and (main_dttm_col := getattr(datasource, "main_dttm_col", None))
- in temporal_columns
+ and (bool(query_object.time_range) or not other_temporal_filters)
+ and main_dttm_col in temporal_columns
)
if should_infer_filter_granularity:
# The inferred column supplies the time-filter subject. It must
not be
# treated as an explicitly selected granularity, which would
rewrite
# the x-axis or remove an independent temporal filter below.
query_object.granularity = main_dttm_col
+ # Drop any TEMPORAL_RANGE filter already targeting the same column
+ # (e.g. left over from when it was the physical x-axis) so
helpers.py
+ # doesn't emit the same WHERE condition twice: once from this
+ # inferred granularity's from_dttm/to_dttm, once from the filter.
+ query_object.filter = [
+ filter_
+ for filter_ in query_object.filter
+ if not (
+ filter_["op"] == "TEMPORAL_RANGE"
+ and filter_["col"] == main_dttm_col
+ )
+ ]
Review Comment:
**Suggestion:** When `time_range` is absent but explicit bounds coexist with
a different range filter on `main_dttm_col`, this drops that filter and can
broaden results.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a4da010c21604928b7c487d90479dfb6&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=a4da010c21604928b7c487d90479dfb6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_factory.py
**Line:** 326:333
**Comment:**
*Logic Error: When `time_range` is absent but explicit bounds coexist
with a different range filter on `main_dttm_col`, this drops that filter and
can broaden results.
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%2F41339&comment_hash=5b7cb3e979be8d492b48b8f30ecc68b5c7809d7fb7094a0cc92cdebcad7b9a92&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41339&comment_hash=5b7cb3e979be8d492b48b8f30ecc68b5c7809d7fb7094a0cc92cdebcad7b9a92&reaction=dislike'>๐</a>
##########
superset/common/query_context_factory.py:
##########
@@ -296,25 +296,41 @@ def _apply_granularity( # noqa: C901
if (column["is_dttm"] if isinstance(column, dict) else
column.is_dttm)
}
x_axis = form_data and form_data.get("x_axis")
- temporal_range_filters = [
+ main_dttm_col = getattr(datasource, "main_dttm_col", None)
+ # Filters that would be clobbered by inferring main_dttm_col below: a
+ # TEMPORAL_RANGE filter already on main_dttm_col itself isn't
+ # "independent", it's the same time subject inference would also
+ # pick, and gets deduped once inference runs.
+ other_temporal_filters = [
filter_
for filter_ in query_object.filter
- if filter_["op"] == "TEMPORAL_RANGE"
+ if filter_["op"] == "TEMPORAL_RANGE" and filter_["col"] !=
main_dttm_col
]
should_infer_filter_granularity: bool = (
is_adhoc_column(x_axis) # type: ignore
and query_object.granularity is None
and bool(query_object.from_dttm or query_object.to_dttm)
- and (bool(query_object.time_range) or not temporal_range_filters)
- and (main_dttm_col := getattr(datasource, "main_dttm_col", None))
- in temporal_columns
+ and (bool(query_object.time_range) or not other_temporal_filters)
+ and main_dttm_col in temporal_columns
)
if should_infer_filter_granularity:
# The inferred column supplies the time-filter subject. It must
not be
# treated as an explicitly selected granularity, which would
rewrite
# the x-axis or remove an independent temporal filter below.
query_object.granularity = main_dttm_col
Review Comment:
**Suggestion:** For timeseries charts, the query builder selects and groups
by this inferred column, splitting expression-axis results by the physical
timestamp.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0d5d8ba6787f41b2aba886ceaafd8707&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=0d5d8ba6787f41b2aba886ceaafd8707&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_factory.py
**Line:** 321:321
**Comment:**
*Api Mismatch: For timeseries charts, the query builder selects and
groups by this inferred column, splitting expression-axis results by the
physical timestamp.
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%2F41339&comment_hash=0f0beec9f8a5542d2b73c4bea17e7fe35db0738d818cf550cc792589cc73bb66&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41339&comment_hash=0f0beec9f8a5542d2b73c4bea17e7fe35db0738d818cf550cc792589cc73bb66&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]
