codeant-ai-for-open-source[bot] commented on code in PR #45271: URL: https://github.com/apache/superset/pull/45271#discussion_r4300149934
########## tests/unit_tests/models/groupby_literal_column_test.py: ########## @@ -0,0 +1,288 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +A Custom SQL dimension holding a bare string literal (``'Other'``) must not +reach the ``GROUP BY`` clause as that bare constant. + +PostgreSQL and its relatives reject a non-integer constant there +(``non-integer constant in GROUP BY``), so the chart query fails even though the +same literal is fine in the ``SELECT`` list. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +import sqlglot +from sqlalchemy import create_engine +from sqlalchemy.orm.session import Session +from sqlglot import expressions as exp + +from superset.connectors.sqla.models import SqlaTable + +METRICS = [{"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "total"}] + + +def _adhoc(sql_expression: str, label: str = "bucket") -> dict[str, str]: + return {"label": label, "sqlExpression": sql_expression, "expressionType": "SQL"} + + +def _make_table(session: Session, uri: str) -> SqlaTable: + from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn + from superset.models.core import Database + + SqlaTable.metadata.create_all(session.get_bind()) + return SqlaTable( + database=Database(database_name="db", sqlalchemy_uri=uri), + schema=None, + table_name="orders", + columns=[ + TableColumn(column_name="region", type="TEXT"), + TableColumn(column_name="amount", type="FLOAT"), + TableColumn(column_name="constant_col", type="TEXT", expression="'Other'"), + ], + metrics=[SqlMetric(metric_name="constant_metric", expression="42")], + ) + + [email protected] +def table(session: Session) -> SqlaTable: + return _make_table(session, "postgresql://user:pass@host/db") + + +def _compile_sql(table: SqlaTable, **overrides: Any) -> str: + query_obj: dict[str, Any] = { + "metrics": METRICS, + "is_timeseries": False, + "row_limit": 100, + "filter": [], + "extras": {}, + "orderby": [], + } + query_obj.update(overrides) + sqla_query = table.get_sqla_query(**query_obj).sqla_query + with table.database.get_sqla_engine() as engine: + return str( + sqla_query.compile( + dialect=engine.dialect, compile_kwargs={"literal_binds": True} + ) + ) + + +def _compile(table: SqlaTable, **overrides: Any) -> tuple[exp.Select, str]: + sql = _compile_sql(table, **overrides) + select = sqlglot.parse_one(sql, dialect="postgres") + assert isinstance(select, exp.Select) + return select, sql + + +def _group_keys(select: exp.Select) -> list[str]: + group = select.args.get("group") + return [key.sql(dialect="postgres") for key in group.expressions] if group else [] + + +def _constant_keys(select: exp.Select) -> list[str]: + """GROUP BY keys that are a bare constant (an integer is a select ordinal).""" + group = select.args.get("group") + return [ + key.sql(dialect="postgres") + for key in (group.expressions if group else []) + if isinstance(key.unnest(), (exp.Literal, exp.Null, exp.Boolean)) + ] + + +def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by( + table: SqlaTable, +) -> None: + select, sql = _compile(table, columns=[_adhoc("'Other'"), "region"]) + + # The literal dimension is still selected under its label ... + assert "bucket" in select.named_selects, sql + + # ... but it must not be a GROUP BY key on its own: only the real dimension + # groups the rows. + group = select.args.get("group") + group_keys = group.expressions if group else [] + bare_constants = [ + key.sql(dialect="postgres") + for key in group_keys + if isinstance(key.unnest(), (exp.Literal, exp.Null, exp.Boolean)) + and not key.unnest().is_int + ] + assert not bare_constants, f"bare constant in GROUP BY: {bare_constants}\n{sql}" + assert any( + isinstance(key, exp.Column) and key.name == "region" for key in group_keys + ), sql + + [email protected]( + "expression", + ["'Other'", "('Other')", "3", "1.5", "NULL", "TRUE"], +) +def test_constant_custom_sql_dimension_is_omitted_from_group_by( + table: SqlaTable, expression: str +) -> None: + select, sql = _compile(table, columns=[_adhoc(expression), "region"]) + assert select.named_selects == ["bucket", "region", "total"], sql + assert _group_keys(select) == ["region"], sql + + +def test_constant_calculated_column_is_omitted_from_group_by( + table: SqlaTable, +) -> None: + select, sql = _compile(table, columns=["constant_col", "region"]) + assert "constant_col" in select.named_selects, sql + assert _group_keys(select) == ["region"], sql + + +def test_constant_raw_string_dimension_is_omitted_from_group_by( + table: SqlaTable, +) -> None: + select, sql = _compile(table, columns=["'Other'", "region"]) + assert _group_keys(select) == ["region"], sql + + [email protected]( + "expression", + [ + "CASE WHEN 1=1 THEN 'x' END", + "CAST('x' AS VARCHAR)", + "'a' || region", + "UPPER('x')", + ], +) +def test_expression_containing_a_constant_stays_a_group_by_key( + table: SqlaTable, expression: str +) -> None: + select, sql = _compile(table, columns=[_adhoc(expression), "region"]) + keys = _group_keys(select) + assert len(keys) == 2, sql + assert keys[-1] == "region", sql + + +def test_group_by_is_unchanged_without_constant_keys(table: SqlaTable) -> None: + select, sql = _compile(table, columns=["region"]) + assert _group_keys(select) == ["region"], sql + + +def test_all_constant_keys_with_a_metric_keep_group_by(table: SqlaTable) -> None: + select, sql = _compile(table, columns=[_adhoc("'Other'")]) + assert _group_keys(select) == ["'Other'"], sql Review Comment: **Suggestion:** `_compile` only checks generated SQL, so this assertion passes with `GROUP BY 'Other'`, which PostgreSQL rejects; charts with only a constant dimension still fail. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Incomplete implementation` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a7e2564d90374d0b895c8f1367a0f517&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=a7e2564d90374d0b895c8f1367a0f517&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/models/groupby_literal_column_test.py **Line:** 186:186 **Comment:** *Incomplete Implementation: `_compile` only checks generated SQL, so this assertion passes with `GROUP BY 'Other'`, which PostgreSQL rejects; charts with only a constant dimension still fail. 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%2F45271&comment_hash=bbb0ff3d724a2abd16c50e3ba5fbc3eca9a8ba0a83d1f786847a4bc1f8bbb96d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45271&comment_hash=bbb0ff3d724a2abd16c50e3ba5fbc3eca9a8ba0a83d1f786847a4bc1f8bbb96d&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]
