codeant-ai-for-open-source[bot] commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3525368078
##########
superset/models/helpers.py:
##########
@@ -2637,21 +2637,50 @@ def filter_values_handler( # pylint:
disable=too-many-arguments # noqa: C901
if values is None:
return None
- def handle_single_value(value: Optional[FilterValue]) ->
Optional[FilterValue]:
- if operator == utils.FilterOperator.TEMPORAL_RANGE:
- return value
+ temporal_comparison_operators = {
+ utils.FilterOperator.EQUALS,
+ utils.FilterOperator.NOT_EQUALS,
+ utils.FilterOperator.IN,
+ utils.FilterOperator.NOT_IN,
+ utils.FilterOperator.GREATER_THAN,
+ utils.FilterOperator.LESS_THAN,
+ utils.FilterOperator.GREATER_THAN_OR_EQUALS,
+ utils.FilterOperator.LESS_THAN_OR_EQUALS,
+ }
+
+ def handle_temporal_value(value: FilterValue) -> FilterValue |
ColumnElement:
if (
- isinstance(value, (float, int))
- and target_generic_type == utils.GenericDataType.TEMPORAL
- and target_native_type is not None
- and db_engine_spec is not None
+ operator not in temporal_comparison_operators
+ or target_generic_type != utils.GenericDataType.TEMPORAL
+ or target_native_type is None
+ or db_engine_spec is None
):
- value = db_engine_spec.convert_dttm(
- target_type=target_native_type,
- dttm=datetime.utcfromtimestamp(value / 1000),
- db_extra=db_extra,
+ return value
+
+ if isinstance(value, (float, int)) and not isinstance(value, bool):
+ epoch_ms: float = value
+ elif isinstance(value, str) and re.fullmatch(r"[+-]?\d+", value):
+ epoch_ms = int(value)
Review Comment:
**Suggestion:** The temporal coercion currently treats any all-digit string
as epoch milliseconds and divides by 1000, which silently mis-converts non-ms
numeric temporal values (for example epoch-seconds or compact date codes like
YYYYMMDD) into incorrect 1970-era timestamps. Restrict coercion to values that
are clearly epoch-millisecond payloads (for example by validating expected
magnitude/length) so only the intended drill/cross-filter format is
transformed. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Chart data API returns rows for wrong timestamps.
- ❌ Drill-to-detail on temporal columns shows incorrect records.
- ⚠️ Cross-filter dashboard interactions mis-filter by shifted dates.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Python, import ExploreMixin from `superset.models.helpers` (class
definition at
`superset/models/helpers.py:1060-1119`) and a temporal engine spec such as
BigQueryEngineSpec from `superset.db_engine_specs.bigquery`.
2. Call `ExploreMixin.filter_values_handler` directly with a temporal column
configuration
but a numeric string that is not epoch milliseconds, for example:
`value = ExploreMixin.filter_values_handler(values="1778630400",
operator=FilterOperator.EQUALS,
target_generic_type=GenericDataType.TEMPORAL,
target_native_type="DATE", db_engine_spec=BigQueryEngineSpec)` (the
function is defined
at `superset/models/helpers.py:2626-2719`).
3. Inside `filter_values_handler`, `handle_single_value()` (lines 82-110 in
`superset/models/helpers.py:2600-2719`) sees a non-empty string, trims it,
and for
temporal columns calls `handle_temporal_value(value)` (line 105) before
building the
comparison filter.
4. In `handle_temporal_value()` (lines 52-80 of the same file), the operator
is EQUALS and
the target type TEMPORAL, so the guard at lines 53-58 passes. The string
`"1778630400"`
matches `re.fullmatch(r"[+-]?\d+", value)` at line 63, so `epoch_ms =
int(value)` at line
64 treats it as milliseconds. `datetime.fromtimestamp(epoch_ms / 1000,
tz=timezone.utc)`
at lines 68-71 interpre seconds as milliseconds, producing a 1970-era
timestamp.
`db_engine_spec.convert_dttm()` then turns this wrong timestamp into a
DATE/TIMESTAMP
literal, and when ExploreMixin later builds the SQL filter in
`get_sqla_query` (filter
loop at `superset/models/helpers.py:3520-3639`), the query runs with the
incorrect
temporal literal, silently filtering the chart-data or drill-to-detail
request to the
wrong time period.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d70a2ec777cc4c1689a4ac38cd71690b&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=d70a2ec777cc4c1689a4ac38cd71690b&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:** 2662:2663
**Comment:**
*Incorrect Condition Logic: The temporal coercion currently treats any
all-digit string as epoch milliseconds and divides by 1000, which silently
mis-converts non-ms numeric temporal values (for example epoch-seconds or
compact date codes like YYYYMMDD) into incorrect 1970-era timestamps. Restrict
coercion to values that are clearly epoch-millisecond payloads (for example by
validating expected magnitude/length) so only the intended drill/cross-filter
format is transformed.
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%2F40180&comment_hash=e8d06c540214df5aa973997aee19101bb16c8b5ba5818c9fb3987383612a009c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40180&comment_hash=e8d06c540214df5aa973997aee19101bb16c8b5ba5818c9fb3987383612a009c&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]
