You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ysinghc (via GitHub)" <gi...@apache.org> on 2025/10/26 11:54:15 UTC

[PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

ysinghc opened a new pull request, #35848:
URL: https://github.com/apache/superset/pull/35848

   fix(db2): use DATE() function for DAY time grain to fix sqlglot parsing error
   
   ### SUMMARY
   Fixes the DB2 DAY time grain expression to use the standard `DATE()` function instead of complex timestamp arithmetic that could not be parsed by sqlglot. This resolves chart creation failures when applying daily time grains to timestamp columns in DB2 databases.
   
   **Problem:**
   The previous implementation used an invalid SQL expression for the DAY time grain:
   ```sql
   CAST({col} as TIMESTAMP) - HOUR({col}) HOURS - MINUTE({col}) MINUTES - SECOND({col}) SECONDS - MICROSECOND({col}) MICROSECONDS
   ```
   
   This syntax could not be parsed by sqlglot, causing `ParseError` exceptions and preventing users from creating charts with daily aggregations on DB2 databases.
   
   **Solution:**
   Changed the `TimeGrain.DAY` expression to use the simpler and more standard:
   ```sql
   DATE({col})
   ```
   
   This approach:
   - Is valid DB2 SQL syntax that sqlglot can parse correctly
   - Is more efficient and cleaner than the previous implementation
   - Follows the same pattern used by MySQL's engine spec
   - Maintains compatibility with all DB2 versions
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   N/A - Backend fix for SQL generation
   
   ### TESTING INSTRUCTIONS
   1. Connect Superset to a DB2 database with a table containing timestamp columns
   2. Create a chart using a timestamp column
   3. Apply a "Day" time grain to the timestamp column
   4. Verify the chart loads successfully without ParseError
   5. Run unit tests: `pytest test_db2.py -v`
   6. Verify all 14 tests pass, including the new `test_time_grain_day_parseable` test
   
   ### ADDITIONAL INFORMATION
   - [x] Has associated issue: Fixes #35792 
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   **Changes:**
   - Modified db2.py - Updated `TimeGrain.DAY` expression
   - Enhanced test_db2.py - Added comprehensive time grain tests including sqlglot parsing validation
   
   **Test Coverage:**
   - Added `test_time_grain_expressions` - Validates all time grain SQL expressions
   - Added `test_time_grain_day_parseable` - Specifically tests that DAY grain produces parseable SQL
   - All existing DB2 tests continue to pass
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448461197

   <details open>
     <summary><strong>Interaction Diagram by <a href="https://bito.ai#sequence_diagram">Bito</a></strong></summary>
   
   ```mermaid
   sequenceDiagram
   participant Test as Test Framework
   participant Db2 as Db2EngineSpec<br/>πŸ”„ Updated | ●●● High
   participant TimeGrains as _time_grain_expressions<br/>πŸ”„ Updated | ●●● High
   participant Base as BaseEngineSpec
   participant Query as Query Generator
   participant DB as Database
   Note over Db2, TimeGrains: DAY grain simplified from CAST to DATE()
   Test->>Db2: test_time_grain_expressions()
   Db2->>TimeGrains: Access _time_grain_expressions[TimeGrain.DAY]
   TimeGrains-->>Db2: Return 'DATE(&#123;col&#125;)'
   Test->>TimeGrains: Format expression with col parameter
   Test->>Db2: test_time_grain_day_parseable()
   Db2->>TimeGrains: Get DAY expression
   Base->>TimeGrains: get_time_grain_expressions() copies dict
   Query->>Base: construct_timestamp_expression()
   Base->>Query: Use simplified DATE expression
   Query->>DB: Execute query with DATE function
   ```
   Critical path: Test Framework-&gt;Db2EngineSpec-&gt;_time_grain_expressions-&gt;BaseEngineSpec-&gt;Query Generator-&gt;Database
   
   
   > **Note:** The DAY time grain expression in Db2EngineSpec was simplified from a complex CAST with multiple time subtractions to a simple DATE() function. This change improves SQL parseability and is validated by new parametrized tests that verify all time grain expressions and ensure the DAY expression can be parsed by sqlglot.
   
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448510493

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448556461

   implemented the suggestions from the bot


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3452491527

   You are right the date({col}) function is much clearer, i assume the current version was for maximum compatibility, to work on almost any version or platform of DB2.
   
   I can apply the modern trunc_timestamp fucntion for HOUR, MINUTE and SECONDS. It is the standard function for this operation in modern versions of db2.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448453685

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35848:
URL: https://github.com/apache/superset/pull/35848#discussion_r2470431757


##########
tests/unit_tests/db_engine_specs/test_db2.py:
##########
@@ -78,3 +81,70 @@ def test_get_prequeries(mocker: MockerFixture) -> None:
     assert Db2EngineSpec.get_prequeries(database, schema="my_schema") == [
         'set current_schema "my_schema"'
     ]
+
+
+@pytest.mark.parametrize(
+    ("grain", "expected_expression"),
+    [
+        (None, "my_col"),
+        (
+            TimeGrain.SECOND,
+            "CAST(my_col as TIMESTAMP) - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.MINUTE,
+            "CAST(my_col as TIMESTAMP)"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.HOUR,
+            "CAST(my_col as TIMESTAMP)"
+            " - MINUTE(my_col) MINUTES"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS ",

Review Comment:
   The expected expression for `TimeGrain.HOUR` includes a trailing space at the end of the string (`" - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS "`). This matches the current implementation in `superset/db_engine_specs/db2.py` line 50, but the trailing space appears to be unintentional. Consider removing the trailing space from both the source code and this test to improve consistency and maintainability.
   ```suggestion
               " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
   ```



##########
tests/unit_tests/db_engine_specs/test_db2.py:
##########
@@ -78,3 +81,70 @@ def test_get_prequeries(mocker: MockerFixture) -> None:
     assert Db2EngineSpec.get_prequeries(database, schema="my_schema") == [
         'set current_schema "my_schema"'
     ]
+
+
+@pytest.mark.parametrize(
+    ("grain", "expected_expression"),
+    [
+        (None, "my_col"),
+        (
+            TimeGrain.SECOND,
+            "CAST(my_col as TIMESTAMP) - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.MINUTE,
+            "CAST(my_col as TIMESTAMP)"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.HOUR,
+            "CAST(my_col as TIMESTAMP)"
+            " - MINUTE(my_col) MINUTES"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS ",
+        ),
+        (TimeGrain.DAY, "DATE(my_col)"),
+        (TimeGrain.WEEK, "my_col - (DAYOFWEEK(my_col)) DAYS"),
+        (TimeGrain.MONTH, "my_col - (DAY(my_col)-1) DAYS"),
+        (
+            TimeGrain.QUARTER,
+            "my_col - (DAY(my_col)-1) DAYS"
+            " - (MONTH(my_col)-1) MONTHS + ((QUARTER(my_col)-1) * 3) MONTHS",
+        ),
+        (
+            TimeGrain.YEAR,
+            "my_col - (DAY(my_col)-1) DAYS - (MONTH(my_col)-1) MONTHS",
+        ),
+    ],
+)
+def test_time_grain_expressions(grain: TimeGrain, expected_expression: str) -> None:

Review Comment:
   The type hint for `grain` parameter should be `TimeGrain | None` or `Optional[TimeGrain]` since one of the test cases passes `None` as the value (line 89).



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3472524822

   LGTM!


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3457872065

   I will try to remove these commits by the copilot and then apply any changes locally and test them before pushing them in the pr.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448458957

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35848?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 50.32%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bb16c8a`](https://app.codecov.io/gh/apache/superset/commit/bb16c8a5a42e5dcbd433a7aafe1a40ebfa3569c6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 2791 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #35848       +/-   ##
   ===========================================
   - Coverage   60.48%   50.32%   -10.17%     
   ===========================================
     Files        1931      600     -1331     
     Lines       76236    44009    -32227     
     Branches     8568     4767     -3801     
   ===========================================
   - Hits        46114    22147    -23967     
   + Misses      28017    21238     -6779     
   + Partials     2105      624     -1481     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `45.90% <ΓΈ> (-3.25%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.57% <ΓΈ> (-4.23%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.31% <ΓΈ> (-13.19%)` | :arrow_down: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/35848/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (+42.36%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/35848?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35848:
URL: https://github.com/apache/superset/pull/35848#discussion_r2472584997


##########
tests/unit_tests/db_engine_specs/test_db2.py:
##########
@@ -78,3 +81,70 @@ def test_get_prequeries(mocker: MockerFixture) -> None:
     assert Db2EngineSpec.get_prequeries(database, schema="my_schema") == [
         'set current_schema "my_schema"'
     ]
+
+
+@pytest.mark.parametrize(
+    ("grain", "expected_expression"),
+    [
+        (None, "my_col"),
+        (
+            TimeGrain.SECOND,
+            "CAST(my_col as TIMESTAMP) - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.MINUTE,
+            "CAST(my_col as TIMESTAMP)"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.HOUR,
+            "CAST(my_col as TIMESTAMP)"
+            " - MINUTE(my_col) MINUTES"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS ",
+        ),
+        (TimeGrain.DAY, "DATE(my_col)"),
+        (TimeGrain.WEEK, "my_col - (DAYOFWEEK(my_col)) DAYS"),
+        (TimeGrain.MONTH, "my_col - (DAY(my_col)-1) DAYS"),
+        (
+            TimeGrain.QUARTER,
+            "my_col - (DAY(my_col)-1) DAYS"
+            " - (MONTH(my_col)-1) MONTHS + ((QUARTER(my_col)-1) * 3) MONTHS",
+        ),
+        (
+            TimeGrain.YEAR,
+            "my_col - (DAY(my_col)-1) DAYS - (MONTH(my_col)-1) MONTHS",
+        ),
+    ],
+)
+def test_time_grain_expressions(grain: TimeGrain, expected_expression: str) -> None:
+    """
+    Test that time grain expressions generate the expected SQL.
+    """
+    from superset.db_engine_specs.db2 import Db2EngineSpec
+
+    actual = Db2EngineSpec._time_grain_expressions[grain].format(col="my_col")
+    assert actual == expected_expression
+
+
+def test_time_grain_day_parseable() -> None:
+    """
+    Test that the DAY time grain expression generates valid SQL
+    that can be parsed by sqlglot.
+
+    This test addresses the bug where the previous expression
+    "CAST({col} as TIMESTAMP) - HOUR({col}) HOURS - ..."
+    could not be parsed by sqlglot.
+    """
+    from superset.db_engine_specs.db2 import Db2EngineSpec
+
+    expression = Db2EngineSpec._time_grain_expressions[TimeGrain.DAY].format(
+        col="my_timestamp_col",
+    )
+    sql = f"SELECT {expression} FROM my_table"  # noqa: S608

Review Comment:
   [nitpick] The `# noqa: S608` comment suppresses a security warning about SQL injection, but this is a false positive since `expression` comes from a static dictionary value, not user input. However, consider using a regular string concatenation or format string without the noqa comment, as the warning suppression is unnecessary and may raise questions during code review. Alternatively, add a comment explaining why the suppression is safe here.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35848:
URL: https://github.com/apache/superset/pull/35848#discussion_r2470563695


##########
tests/unit_tests/db_engine_specs/test_db2.py:
##########
@@ -78,3 +81,70 @@ def test_get_prequeries(mocker: MockerFixture) -> None:
     assert Db2EngineSpec.get_prequeries(database, schema="my_schema") == [
         'set current_schema "my_schema"'
     ]
+
+
+@pytest.mark.parametrize(
+    ("grain", "expected_expression"),
+    [
+        (None, "my_col"),
+        (
+            TimeGrain.SECOND,
+            "CAST(my_col as TIMESTAMP) - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.MINUTE,
+            "CAST(my_col as TIMESTAMP)"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (
+            TimeGrain.HOUR,
+            "CAST(my_col as TIMESTAMP)"
+            " - MINUTE(my_col) MINUTES"
+            " - SECOND(my_col) SECONDS - MICROSECOND(my_col) MICROSECONDS",
+        ),
+        (TimeGrain.DAY, "DATE(my_col)"),
+        (TimeGrain.WEEK, "my_col - (DAYOFWEEK(my_col)) DAYS"),
+        (TimeGrain.MONTH, "my_col - (DAY(my_col)-1) DAYS"),
+        (
+            TimeGrain.QUARTER,
+            "my_col - (DAY(my_col)-1) DAYS"
+            " - (MONTH(my_col)-1) MONTHS + ((QUARTER(my_col)-1) * 3) MONTHS",
+        ),
+        (
+            TimeGrain.YEAR,
+            "my_col - (DAY(my_col)-1) DAYS - (MONTH(my_col)-1) MONTHS",
+        ),
+    ],
+)
+def test_time_grain_expressions(grain: TimeGrain, expected_expression: str) -> None:
+    """
+    Test that time grain expressions generate the expected SQL.
+    """
+    from superset.db_engine_specs.db2 import Db2EngineSpec
+
+    actual = Db2EngineSpec._time_grain_expressions[grain].format(col="my_col")
+    assert actual == expected_expression
+
+
+def test_time_grain_day_parseable() -> None:
+    """
+    Test that the DAY time grain expression generates valid SQL
+    that can be parsed by sqlglot.
+
+    This test addresses the bug where the previous expression
+    "CAST({col} as TIMESTAMP) - HOUR({col}) HOURS - ..."
+    could not be parsed by sqlglot.
+    """
+    from superset.db_engine_specs.db2 import Db2EngineSpec
+
+    expression = Db2EngineSpec._time_grain_expressions[TimeGrain.DAY].format(
+        col="my_timestamp_col",
+    )

Review Comment:
   The `noqa: S608` comment suppresses a security warning for SQL injection. While the expression comes from a trusted source (engine specs), consider using a safer approach like sqlglot's builder API to construct the query, or add a comment explaining why this is safe in this specific test context.
   ```suggestion
       )
       # The interpolated value in this SQL string comes from a trusted, static source
       # (Db2EngineSpec._time_grain_expressions), and this query is only parsed, not executed.
       # Therefore, this usage is safe in this test context.
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3451535806

   Thanks for this! I am trying to understand why the current version is what it is, given that your version is much clearer and shorter. I traced back to the original PR that complicated things: https://github.com/apache/superset/pull/2545 Do you have any guess why they went from `DAY({col})` to:
   
   ```
   CAST({col} as TIMESTAMP)'
                 ' - HOUR({col}) HOURS'
                 ' - MINUTE({col}) MINUTES'
                 ' - SECOND({col}) SECONDS'
                 ' - MICROSECOND({col}) MICROSECONDS ')
   ```
   ? Should the change you made be made also to SECOND, MINUTE, and HOUR which look like they use similar structures? I am unfamiliar with DB2 and would love if someone could explain to me.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35848:
URL: https://github.com/apache/superset/pull/35848#issuecomment-3448510389

   /review


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(db2): update time grain expressions for DAY to use DATE function [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #35848:
URL: https://github.com/apache/superset/pull/35848


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org