codeant-ai-for-open-source[bot] commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3564755768
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -3410,3 +3410,265 @@ def fake_engine(*args, **kwargs):
# ```forecasts.original`.`total_cost``` (the regression), so this negative
# assertion catches the actual failure mode, not just an exact-string
match.
assert "`forecasts.original`" not in sql
+
+
+def test_temporal_epoch_string_filter_is_coerced_for_bigquery() -> None:
+ """
+ Drill-to-detail can send JavaScript timestamp strings for temporal values.
+ BigQuery DATE filters must receive a DATE literal instead of the raw
string.
+ """
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="1778630400000",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert isinstance(value, ColumnElement)
+ assert str(value) == "CAST('2026-05-13' AS DATE)"
+
+
+def test_temporal_negative_epoch_string_filter_is_coerced_for_bigquery() ->
None:
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="-1778630400000",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert isinstance(value, ColumnElement)
+ assert str(value) == "CAST('1913-08-22' AS DATE)"
+
+
[email protected]("value", [1778630400000, 1778630400000.0])
+def test_temporal_numeric_filter_is_coerced_for_bigquery(
+ value: int | float,
+) -> None:
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ result = ExploreMixin.filter_values_handler(
+ values=value,
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert isinstance(result, ColumnElement)
+ assert str(result) == "CAST('2026-05-13' AS DATE)"
+
+
+def test_temporal_overflow_epoch_filter_is_not_coerced() -> None:
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = "999999999999999999999999999999999999999"
+
+ result = ExploreMixin.filter_values_handler(
+ values=value,
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert result == value
+
+
+def test_temporal_epoch_filter_is_not_coerced_without_engine_literal() -> None:
+ from superset.db_engine_specs.base import BaseEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ result = ExploreMixin.filter_values_handler(
+ values="1778630400000",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="UNKNOWN_TYPE",
+ db_engine_spec=BaseEngineSpec,
+ )
+
+ assert result == "1778630400000"
+
+
[email protected](
+ "operator",
+ [FilterOperator.IN, FilterOperator.NOT_IN],
+)
+def test_temporal_epoch_string_filter_list_is_coerced_for_bigquery(
+ operator: FilterOperator,
+) -> None:
+ """
+ Cross-filtering can send JavaScript timestamp strings inside IN lists.
+ BigQuery DATE filters must receive DATE literals instead of raw strings.
+ """
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ values = ExploreMixin.filter_values_handler(
+ values=["1777248000000"],
+ operator=operator,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ is_list_target=True,
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert isinstance(values, list)
+ assert len(values) == 1
+ assert isinstance(values[0], ColumnElement)
+ assert str(values[0]) == "CAST('2026-04-27' AS DATE)"
+
+
+def test_temporal_epoch_string_filter_list_is_coerced_for_clickhouse() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ values = ExploreMixin.filter_values_handler(
+ values=["1777248000000"],
+ operator=FilterOperator.IN,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="Date",
+ is_list_target=True,
+ db_engine_spec=ClickHouseEngineSpec,
+ )
+
+ assert isinstance(values, list)
+ assert len(values) == 1
+ assert isinstance(values[0], ColumnElement)
+ assert str(values[0]) == "toDate('2026-04-27')"
+
+
[email protected](
+ "target_type,expected",
+ [
+ ("DATE", "TO_DATE('2026-05-13', 'YYYY-MM-DD')"),
+ (
+ "TIMESTAMP",
+ "TO_TIMESTAMP('2026-05-13 00:00:00.000000', 'YYYY-MM-DD
HH24:MI:SS.US')",
+ ),
+ ],
+)
+def test_temporal_epoch_string_filter_is_coerced_for_postgres(
+ target_type: str,
+ expected: str,
+) -> None:
+ from superset.db_engine_specs.postgres import PostgresEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="1778630400000",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type=target_type,
+ db_engine_spec=PostgresEngineSpec,
+ )
+
+ assert isinstance(value, ColumnElement)
+ assert str(value) == expected
+
+
+def test_temporal_epoch_string_filter_list_is_coerced_for_postgres() -> None:
+ from superset.db_engine_specs.postgres import PostgresEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ values = ExploreMixin.filter_values_handler(
+ values=["1777248000000"],
+ operator=FilterOperator.IN,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ is_list_target=True,
+ db_engine_spec=PostgresEngineSpec,
+ )
+
+ assert isinstance(values, list)
+ assert len(values) == 1
+ assert isinstance(values[0], ColumnElement)
+ assert str(values[0]) == "TO_DATE('2026-04-27', 'YYYY-MM-DD')"
+
+
+def test_non_temporal_numeric_string_filter_is_not_coerced() -> None:
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="1778630400000",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.STRING,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert value == "1778630400000"
+
+
+def test_temporal_range_filter_value_is_not_coerced() -> None:
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="No filter",
+ operator=FilterOperator.TEMPORAL_RANGE,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type="DATE",
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert value == "No filter"
+
+
[email protected](
+ "target_type,expected",
+ [
+ (
+ "DATETIME",
+ "CAST('2026-05-13T14:30:00.123000' AS DATETIME)",
+ ),
+ (
+ "TIMESTAMP",
+ "CAST('2026-05-13T14:30:00.123000' AS TIMESTAMP)",
+ ),
+ ],
+)
+def test_temporal_epoch_string_filter_is_coerced_for_datetime_bigquery(
+ target_type: str,
+ expected: str,
+) -> None:
+ """Sub-second epoch precision survives DATETIME / TIMESTAMP coercion."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.models.helpers import ExploreMixin
+
+ value = ExploreMixin.filter_values_handler(
+ values="1778682600123",
+ operator=FilterOperator.EQUALS,
+ target_generic_type=GenericDataType.TEMPORAL,
+ target_native_type=target_type,
+ db_engine_spec=BigQueryEngineSpec,
+ )
+
+ assert isinstance(value, ColumnElement)
+ assert str(value) == expected
+
+
+def test_temporal_non_numeric_string_filter_is_not_coerced() -> None:
+ """Only digit strings (JS epoch ms) are coerced — anything else passes
through."""
Review Comment:
**Suggestion:** The new docstring is factually incorrect: coercion is not
limited to digit-only strings because signed numeric strings are also coerced
(as covered by the negative-epoch test and the `[+-]?\d+` matcher). Update the
wording so the test documentation matches actual behavior and does not mislead
future maintainers. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Test docstring misstates signed epoch coercion behavior.
⚠️ Future maintainers may misinterpret filter_values_handler numeric
handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/models/helpers_test.py` and locate
`test_temporal_non_numeric_string_filter_is_not_coerced` added in this PR
(around lines
3661–3674 in the new hunk); note the docstring on line 3662: `Only digit
strings (JS epoch
ms) are coerced — anything else passes through.`.
2. In the same file, inspect
`test_temporal_negative_epoch_string_filter_is_coerced_for_bigquery` (around
lines
3456–3469 in the new hunk), which calls `ExploreMixin.filter_values_handler`
with the
signed string value `"-1778630400000"` and asserts it is coerced into a
BigQuery DATE
literal (`CAST('1913-08-22' AS DATE)`), demonstrating that signed numeric
strings are
treated as temporal epochs.
3. Open `superset/superset/models/helpers.py` and examine
`ExploreMixin.filter_values_handler` (definition starting at line 2626,
confirmed via
Grep); inside `handle_temporal_value` on lines 2641–2644, the code uses
`re.fullmatch(r"[+-]?\d+", value)` to detect temporal epoch strings, meaning
any signed
numeric string (optional `+` or `-` followed by digits) is eligible for
coercion, not just
digit-only strings.
4. Compare the docstring’s statement (“Only digit strings … are coerced”)
with the actual
behavior verified in step 2 and the regex in step 3; the implementation and
tests clearly
show that signed numeric strings are also coerced, so the docstring is
factually incorrect
and can mislead future maintainers about supported timestamp formats.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1692f1d9957742afa85f3313dd18bf22&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=1692f1d9957742afa85f3313dd18bf22&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:** tests/unit_tests/models/helpers_test.py
**Line:** 3662:3662
**Comment:**
*Comment Mismatch: The new docstring is factually incorrect: coercion
is not limited to digit-only strings because signed numeric strings are also
coerced (as covered by the negative-epoch test and the `[+-]?\d+` matcher).
Update the wording so the test documentation matches actual behavior and does
not mislead future maintainers.
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=4b99acba5e6b8b3ed00e32503c81549512a7910b32e323d3f0f7293e9daecc56&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40180&comment_hash=4b99acba5e6b8b3ed00e32503c81549512a7910b32e323d3f0f7293e9daecc56&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]
