codeant-ai-for-open-source[bot] commented on code in PR #42095:
URL: https://github.com/apache/superset/pull/42095#discussion_r3591317742
##########
superset/db_engine_specs/postgres.py:
##########
@@ -49,6 +52,40 @@
logger = logging.getLogger()
+_DATE_TRUNC_UNITS: frozenset[str] = frozenset(
+ {"second", "minute", "hour", "day", "week", "month", "quarter", "year"}
+)
+
+
+def normalize_date_trunc_units(expression: str) -> str:
+ """Lowercase recognized DATE_TRUNC unit literals without regenerating
SQL."""
+ try:
+ tokens = Tokenizer(dialect="postgres").tokenize(expression)
+ except TokenError:
+ return expression
+
+ replacements: list[tuple[int, int, str]] = []
+ for index, (function, left_paren, unit) in enumerate(
+ zip(tokens, tokens[1:], tokens[2:], strict=False)
+ ):
+ normalized_unit = unit.text.lower()
+ raw_literal = expression[unit.start : unit.end + 1]
+ if (
+ function.token_type is TokenType.VAR
+ and function.text.upper() == "DATE_TRUNC"
+ and (index == 0 or tokens[index - 1].token_type is not
TokenType.DOT)
Review Comment:
**Suggestion:** The normalization logic skips any `DATE_TRUNC` call preceded
by a dot, which also excludes valid PostgreSQL built-in calls like
`pg_catalog.DATE_TRUNC(...)`. In those cases uppercase units remain unchanged,
so metric SQL can still diverge from the generated time-grain expression and
trigger grouping failures. Restrict the skip logic to non-built-in dotted names
(or explicitly allow `pg_catalog.DATE_TRUNC`) so schema-qualified built-ins are
normalized too. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ PostgreSQL charts with schema-qualified DATE_TRUNC metrics fail grouping.
- ⚠️ Redshift metrics using pg_catalog.DATE_TRUNC risk similar failures.
- ⚠️ Metric normalization inconsistent for equivalent DATE_TRUNC invocations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Define a SQL metric on a PostgreSQL dataset whose expression uses a
schema-qualified
built-in DATE_TRUNC, for example `pg_catalog.DATE_TRUNC('QUARTER',
created_at)`, so that
the metric path in `SqlMetric.get_sqla_col()` at
`superset/connectors/sqla/models.py:1249-1252` will call
`_normalize_custom_sql_metric(...)` with this expression and the Postgres
engine spec
normalizer.
2. When the chart or explore query runs, `_normalize_custom_sql_metric()` in
`superset/models/helpers.py:835-833` invokes
`PostgresEngineSpec.normalize_custom_sql_metric()` (defined in
`superset/db_engine_specs/postgres.py:316-318`), which forwards the metric
SQL to
`normalize_date_trunc_units()` at
`superset/db_engine_specs/postgres.py:60-87`.
3. Inside `normalize_date_trunc_units()`, the expression
`pg_catalog.DATE_TRUNC('QUARTER',
created_at)` is tokenized by sqlglot (lines 62-63), and the loop at lines
68-70 iterates
over `(function, left_paren, unit)` triples; for the triple corresponding to
the
DATE_TRUNC call, `function` is the `DATE_TRUNC` identifier but `index` is 2
and
`tokens[index - 1]` is the preceding `DOT` token, so the condition `and
(index == 0 or
tokens[index - 1].token_type is not TokenType.DOT)` at line 76 evaluates to
False and no
replacement is added.
4. Because the schema-qualified built-in DATE_TRUNC call is skipped by this
dotted-name
guard, the unit literal remains `'QUARTER'` instead of being normalized to
`'quarter',
even though the engine's time-grain templates in
`PostgresBaseEngineSpec._time_grain_expressions` (see
`superset/db_engine_specs/postgres.py:211-226`) use unqualified
`DATE_TRUNC('quarter',
{col})`, so custom metrics written with `pg_catalog.DATE_TRUNC('QUARTER',
...)` can still
diverge from the generated grouping expression and reintroduce the grouping
inconsistency
that this normalization hook is meant to prevent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec30d5ce2fa24e138689ffe20cba5fee&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=ec30d5ce2fa24e138689ffe20cba5fee&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/db_engine_specs/postgres.py
**Line:** 76:76
**Comment:**
*Incomplete Implementation: The normalization logic skips any
`DATE_TRUNC` call preceded by a dot, which also excludes valid PostgreSQL
built-in calls like `pg_catalog.DATE_TRUNC(...)`. In those cases uppercase
units remain unchanged, so metric SQL can still diverge from the generated
time-grain expression and trigger grouping failures. Restrict the skip logic to
non-built-in dotted names (or explicitly allow `pg_catalog.DATE_TRUNC`) so
schema-qualified built-ins are normalized too.
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%2F42095&comment_hash=ad24e62155fede6bd3c4969080bc0581f845e9eaf20072c9b1376b4e6bbb5c85&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=ad24e62155fede6bd3c4969080bc0581f845e9eaf20072c9b1376b4e6bbb5c85&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]
