codeant-ai-for-open-source[bot] commented on code in PR #42054:
URL: https://github.com/apache/superset/pull/42054#discussion_r3592996593
##########
superset/models/helpers.py:
##########
@@ -1912,6 +1913,19 @@ def processing_time_offsets( # pylint:
disable=too-many-locals,too-many-stateme
outer_from_dttm,
outer_to_dttm,
)
+ elif parse_human_timedelta(offset, outer_from_dttm) ==
timedelta():
+ # get_past_or_future silently returns the source time
+ # for offsets it cannot parse; querying with an
+ # unshifted window would present the current period's
+ # data as the comparison series.
+ raise QueryObjectValidationError(
+ _(
+ "Unable to interpret the time offset: "
+ "%(offset)s. Use a relative time such as "
+ '"1 month ago".',
+ offset=offset,
+ )
+ )
Review Comment:
**Suggestion:** The validation treats any offset whose parsed delta equals
zero as “uninterpretable”, which incorrectly rejects valid offsets such as `0
days ago` (and other expressions that legitimately resolve to no shift for the
chosen reference time). This turns a parseable offset into a hard 400 error;
instead of using `== timedelta()` as the parseability test, validate by
checking whether parsing failed/raised or by using a parser signal that
distinguishes “unparseable” from “parsed to zero”. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Relative time comparison fails for zero-shift offsets.
- ⚠️ Users cannot request no-change comparison periods.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a QueryObject with a time comparison offset that legitimately
resolves to zero
delta (for example, "0 days ago"), so that `query_object.time_offsets`
contains ["0 days
ago"] (see superset/common/query_object.py:107-107, where `time_offsets:
list[str]` is
defined).
2. Execute a chart query that uses this QueryObject via a datasource; in
superset/models/helpers.py:1803-1810 the datasource calls
`self.processing_time_offsets(df, query_object, ...)` when
`query_object.time_offsets` is
non-empty, entering `processing_time_offsets` at
superset/models/helpers.py:1828.
3. Inside `processing_time_offsets`, for the offset "0 days ago" the code
falls into the
relative-offset branch at superset/models/helpers.py:1888-1907 (the `else`
block labeled
"RELATIVE OFFSET LOGIC"), where `self.is_valid_date(offset)` is False and
`offset !=
"inherit"`, so execution reaches the `elif parse_human_timedelta(offset,
outer_from_dttm)
== timedelta():` guard at lines 1916-1921.
4. The guard calls `parse_human_timedelta(offset, outer_from_dttm)`; per
superset/utils/date_parser.py:141-154, `parse_human_timedelta` computes
`get_past_or_future(human_readable, source_time) - source_dttm`, and for
phrases like "0
days ago" parsedatetime returns the same `source_dttm`, yielding
`timedelta(0)`. Because
the code compares this result to `timedelta()` and treats equality as an
"uninterpretable"
offset, it raises `QueryObjectValidationError` with message "Unable to
interpret the time
offset..." (helpers.py:1916-1928), causing the time-comparison request to
fail with a 400
error even though the offset string was parsed successfully to a legitimate
zero-shift
delta.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d6167cc6e44a4cc29f5396c09a60eb94&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=d6167cc6e44a4cc29f5396c09a60eb94&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/models/helpers.py
**Line:** 1916:1928
**Comment:**
*Incorrect Condition Logic: The validation treats any offset whose
parsed delta equals zero as “uninterpretable”, which incorrectly rejects valid
offsets such as `0 days ago` (and other expressions that legitimately resolve
to no shift for the chosen reference time). This turns a parseable offset into
a hard 400 error; instead of using `== timedelta()` as the parseability test,
validate by checking whether parsing failed/raised or by using a parser signal
that distinguishes “unparseable” from “parsed to zero”.
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%2F42054&comment_hash=6e4da12217f6ad41c8ca92cbddd6fb191c44c4938219d059ca3dfd5b8e53ac0b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42054&comment_hash=6e4da12217f6ad41c8ca92cbddd6fb191c44c4938219d059ca3dfd5b8e53ac0b&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]
