codeant-ai-for-open-source[bot] commented on code in PR #38617:
URL: https://github.com/apache/superset/pull/38617#discussion_r2929410401
##########
tests/unit_tests/db_engine_specs/test_mysql.py:
##########
@@ -262,3 +264,81 @@ def test_column_type_mutator(
mock_cursor.description = description
assert spec.fetch_data(mock_cursor) == expected_result
+
+
[email protected](
+ ("grain", "expected_expression"),
+ [
+ (None, "my_col"),
+ (
+ TimeGrain.SECOND,
+ "DATE_ADD(DATE(my_col), "
+ "INTERVAL (HOUR(my_col)*60*60 + MINUTE(my_col)*60"
+ " + SECOND(my_col)) SECOND)",
+ ),
+ (
+ TimeGrain.MINUTE,
+ "DATE_ADD(DATE(my_col), "
+ "INTERVAL (HOUR(my_col)*60 + MINUTE(my_col)) MINUTE)",
+ ),
+ (
+ TimeGrain.HOUR,
+ "DATE_ADD(CAST(DATE(my_col) AS DATETIME), INTERVAL HOUR(my_col)
HOUR)",
+ ),
+ (TimeGrain.DAY, "DATE(my_col)"),
+ (
+ TimeGrain.WEEK,
+ "DATE(DATE_SUB(my_col, INTERVAL DAYOFWEEK(my_col) - 1 DAY))",
+ ),
+ (
+ TimeGrain.MONTH,
+ "DATE(DATE_SUB(my_col, INTERVAL DAYOFMONTH(my_col) - 1 DAY))",
+ ),
+ (
+ TimeGrain.QUARTER,
+ "MAKEDATE(YEAR(my_col), 1) "
+ "+ INTERVAL QUARTER(my_col) QUARTER - INTERVAL 1 QUARTER",
+ ),
+ (
+ TimeGrain.YEAR,
+ "DATE(DATE_SUB(my_col, INTERVAL DAYOFYEAR(my_col) - 1 DAY))",
+ ),
+ (
+ TimeGrain.WEEK_STARTING_MONDAY,
+ "DATE(DATE_SUB(my_col, "
+ "INTERVAL DAYOFWEEK(DATE_SUB(my_col, "
+ "INTERVAL 1 DAY)) - 1 DAY))",
+ ),
+ ],
+)
+def test_time_grain_expressions(
+ grain: Optional[TimeGrain], expected_expression: str
+) -> None:
+ """
+ Test that MySQL time grain expression templates produce the expected SQL.
+ Guards against the DATE() function being dropped in the HOUR grain.
+ """
+ from superset.db_engine_specs.mysql import MySQLEngineSpec
+
+ actual = MySQLEngineSpec._time_grain_expressions[grain].replace("{col}",
"my_col")
+ assert actual == expected_expression
+
+
+def test_compile_timegrain_expression_preserves_date_function() -> None:
+ """
+ Test that compile_timegrain_expression preserves the DATE() wrapper in the
+ MySQL HOUR time grain expression.
+
+ Regression test for: ECharts HOUR grain generates invalid SQL (DATE()
dropped).
+ """
+ from superset.db_engine_specs.mysql import MySQLEngineSpec
+
+ col = column("my_col")
+ template = MySQLEngineSpec._time_grain_expressions[TimeGrain.HOUR]
+ expr = TimestampExpression(template, col)
+
+ compiled = str(expr)
+ assert (
+ compiled
+ == "DATE_ADD(CAST(DATE(my_col) AS DATETIME), INTERVAL HOUR(my_col)
HOUR)"
+ ), f"DATE() wrapper was dropped or CAST lost. Got: {compiled}"
Review Comment:
**Suggestion:** The regression test compiles `TimestampExpression` directly,
but the original bug happens when the expression is proxied through a subquery
(series-limit path). As written, this test can pass even if the proxy path
still strips `DATE(...)`, so it does not reliably guard against the real
regression. Compile the expression through a subquery and assert the wrapped
`CAST(DATE(...))` is still present there. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Regression may bypass tests in series-limit SQL path.
- ⚠️ MySQL HOUR bucketing bug can reappear unnoticed.
- ⚠️ Affects chart SQL from Chart Data API.
```
</details>
```suggestion
from sqlalchemy import select
compiled = str(select(select(expr.label("bucket")).subquery().c.bucket))
assert "DATE_ADD(CAST(DATE(my_col) AS DATETIME), INTERVAL HOUR(my_col)
HOUR)" in compiled, (
f"DATE() wrapper was dropped or CAST lost in proxied expression.
Got: {compiled}"
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger chart query generation through real entrypoint
`superset/charts/data/api.py:160-167`, where `ChartDataCommand` executes
chart SQL
generation for Explore/ECharts requests.
2. Follow runtime call chain: `ChartDataCommand.run()`
(`superset/commands/chart/data/get_data_command.py:40-48`) →
`QueryContext.get_payload()`
(`superset/common/query_context.py:93-99`) →
`QueryContextProcessor.get_query_result()`
(`superset/common/query_context_processor.py:235-243`) → datasource
`get_sqla_query()`
(`superset/models/helpers.py:2630+`).
3. Use a timeseries query with `series_limit` enabled; code enters
subquery/proxy path at
`superset/models/helpers.py:3272-3370`, where groupby expressions are
wrapped, aliased,
and proxied via `subq.alias(...)`.
4. Current test `tests/unit_tests/db_engine_specs/test_mysql.py:327-344`
only compiles
`str(expr)` directly, never through `.subquery().c...`; therefore it cannot
detect
regressions specific to proxied expressions, despite
`TimestampExpression._constructor`
existing explicitly for proxy behavior
(`superset/db_engine_specs/base.py:26-30`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_mysql.py
**Line:** 340:344
**Comment:**
*Logic Error: The regression test compiles `TimestampExpression`
directly, but the original bug happens when the expression is proxied through a
subquery (series-limit path). As written, this test can pass even if the proxy
path still strips `DATE(...)`, so it does not reliably guard against the real
regression. Compile the expression through a subquery and assert the wrapped
`CAST(DATE(...))` is still present there.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38617&comment_hash=b7a59f33a3e65dc4b0bea65650da7bc23faaedc3397338a418575cbf16b0bd1b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38617&comment_hash=b7a59f33a3e65dc4b0bea65650da7bc23faaedc3397338a418575cbf16b0bd1b&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]
