codeant-ai-for-open-source[bot] commented on code in PR #38174: URL: https://github.com/apache/superset/pull/38174#discussion_r2842536378
########## superset/db_engine_specs/odps.py: ########## @@ -0,0 +1,192 @@ +# 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. +from __future__ import annotations + +import logging +from typing import Any, Optional, TYPE_CHECKING + +from sqlalchemy import select, text +from sqlalchemy.engine.base import Engine + +from superset.databases.schemas import ( + TableMetadataColumnsResponse, + TableMetadataResponse, +) +from superset.databases.utils import ( + get_col_type, + get_foreign_keys_metadata, + get_indexes_metadata, +) +from superset.db_engine_specs.base import BaseEngineSpec, BasicParametersMixin +from superset.sql.parse import Partition, SQLScript, Table +from superset.superset_typing import ResultSetColumnType + +if TYPE_CHECKING: + from superset.models.core import Database + +logger = logging.getLogger(__name__) + + +class OdpsBaseEngineSpec(BaseEngineSpec): + @classmethod + def get_table_metadata( + cls, + database: Database, + table: Table, + partition: Optional[Partition] = None, + ) -> TableMetadataResponse: + """ + Returns basic table metadata + :param database: Database instance + :param table: A Table instance + :param partition: A Table partition info + :return: Basic table metadata + """ + return cls.get_table_metadata(database, table, partition) + + +class OdpsEngineSpec(BasicParametersMixin, OdpsBaseEngineSpec): + engine = "odps" + engine_name = "ODPS (MaxCompute)" + default_driver = "odps" + + @classmethod + def get_table_metadata( + cls, database: Any, table: Table, partition: Optional[Partition] = None + ) -> TableMetadataResponse: + """ + Get table metadata information, including type, pk, fks. + This function raises SQLAlchemyError when a schema is not found. + + :param partition: The table's partition info + :param database: The database model + :param table: Table instance + :return: Dict table metadata ready for API response + """ + keys = [] + columns = database.get_columns(table) + primary_key = database.get_pk_constraint(table) + if primary_key and primary_key.get("constrained_columns"): + primary_key["column_names"] = primary_key.pop("constrained_columns") + primary_key["type"] = "pk" + keys += [primary_key] + foreign_keys = get_foreign_keys_metadata(database, table) + indexes = get_indexes_metadata(database, table) + keys += foreign_keys + indexes + payload_columns: list[TableMetadataColumnsResponse] = [] + table_comment = database.get_table_comment(table) + for col in columns: + dtype = get_col_type(col) + payload_columns.append( + { + "name": col["column_name"], + "type": dtype.split("(")[0] if "(" in dtype else dtype, + "longType": dtype, + "keys": [ + k for k in keys if col["column_name"] in k["column_names"] + ], + "comment": col.get("comment"), + } + ) + + with database.get_sqla_engine( + catalog=table.catalog, schema=table.schema + ) as engine: + return { + "name": table.table, + "columns": payload_columns, + "selectStar": cls.select_star( + database=database, + table=table, + engine=engine, + limit=100, + show_cols=False, + indent=True, + latest_partition=True, + cols=columns, + partition=partition, + ), + "primaryKey": primary_key, + "foreignKeys": foreign_keys, + "indexes": keys, + "comment": table_comment, + } + + @classmethod + def select_star( # pylint: disable=too-many-arguments + cls, + database: Database, + table: Table, + engine: Engine, + limit: int = 100, + show_cols: bool = False, + indent: bool = True, + latest_partition: bool = True, + cols: list[ResultSetColumnType] | None = None, + partition: Optional[Partition] = None, + ) -> str: + """ + Generate a "SELECT * from [schema.]table_name" query with appropriate limit. + + WARNING: expects only unquoted table and schema names. + + :param partition: The table's partition info + :param database: Database instance + :param table: Table instance + :param engine: SqlAlchemy Engine instance + :param limit: limit to impose on query + :param show_cols: Show columns in query; otherwise use "*" + :param indent: Add indentation to query + :param latest_partition: Only query the latest partition + :param cols: Columns to include in query + :return: SQL query + """ + # pylint: disable=redefined-outer-name + fields: str | list[Any] = "*" + cols = cols or [] + if (show_cols or latest_partition) and not cols: + cols = database.get_columns(table) + + if show_cols: + fields = cls._get_fields(cols) + full_table_name = cls.quote_table(table, engine.dialect) Review Comment: **Suggestion:** `OdpsEngineSpec.select_star` assumes its third argument `engine` always has a `.dialect` attribute, but in the generic code path `Database.select_star` passes a SQLAlchemy `Dialect` object instead, causing an `AttributeError`; the method should accept either an engine or a dialect and normalize to a dialect before calling `quote_table`. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ /api/v1/database/<pk>/select_star fails for ODPS. - ❌ SQLLab cannot generate SELECT * for ODPS tables. - ⚠️ ODPS engine diverges from BaseEngineSpec.select_star contract. ``` </details> ```suggestion dialect = getattr(engine, "dialect", engine) full_table_name = cls.quote_table(table, dialect) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure an ODPS (MaxCompute) database and ensure its backend is `"odps"`, so `Database.db_engine_spec` resolves to `OdpsEngineSpec` (see `superset/models/core.py:1021-1038` and `superset/db_engine_specs/odps.py:62-65`). 2. Trigger the `/api/v1/database/<pk>/select_star/<table_name>/` endpoint defined in `DatabaseRestApi.select_star` (`superset/databases/api.py:1181-1242`) for this ODPS database, for example from the UI "select star" feature in SQLLab. 3. Inside `DatabaseRestApi.select_star`, Superset calls `database.select_star(Table(table_name, schema_name, database.get_default_catalog()), latest_partition=True)` (`superset/databases/api.py:1232-1237`). 4. `Database.select_star` (`superset/models/core.py:825-845`) computes `dialect = self.get_dialect()` and then calls `self.db_engine_spec.select_star(self, table, dialect=dialect, limit=..., show_cols=..., indent=..., latest_partition=..., cols=cols)`. Because `OdpsEngineSpec.select_star` is defined with signature `(database, table, engine, ...)` and no `dialect` keyword (`superset/db_engine_specs/odps.py:129-140`), Python raises `TypeError: select_star() got an unexpected keyword argument 'dialect'` before reaching the `engine.dialect` access at lines 166-167. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/db_engine_specs/odps.py **Line:** 166:166 **Comment:** *Logic Error: `OdpsEngineSpec.select_star` assumes its third argument `engine` always has a `.dialect` attribute, but in the generic code path `Database.select_star` passes a SQLAlchemy `Dialect` object instead, causing an `AttributeError`; the method should accept either an engine or a dialect and normalize to a dialect before calling `quote_table`. 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%2F38174&comment_hash=2f4fe45fa0bebbe531edb038161caa1133af27c89d8ca76888700edea5037f00&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38174&comment_hash=2f4fe45fa0bebbe531edb038161caa1133af27c89d8ca76888700edea5037f00&reaction=dislike'>👎</a> ########## superset/db_engine_specs/odps.py: ########## @@ -0,0 +1,192 @@ +# 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. +from __future__ import annotations + +import logging +from typing import Any, Optional, TYPE_CHECKING + +from sqlalchemy import select, text +from sqlalchemy.engine.base import Engine + +from superset.databases.schemas import ( + TableMetadataColumnsResponse, + TableMetadataResponse, +) +from superset.databases.utils import ( + get_col_type, + get_foreign_keys_metadata, + get_indexes_metadata, +) +from superset.db_engine_specs.base import BaseEngineSpec, BasicParametersMixin +from superset.sql.parse import Partition, SQLScript, Table +from superset.superset_typing import ResultSetColumnType + +if TYPE_CHECKING: + from superset.models.core import Database + +logger = logging.getLogger(__name__) + + +class OdpsBaseEngineSpec(BaseEngineSpec): + @classmethod + def get_table_metadata( + cls, + database: Database, + table: Table, + partition: Optional[Partition] = None, + ) -> TableMetadataResponse: + """ + Returns basic table metadata + :param database: Database instance + :param table: A Table instance + :param partition: A Table partition info + :return: Basic table metadata + """ + return cls.get_table_metadata(database, table, partition) Review Comment: **Suggestion:** The `OdpsBaseEngineSpec.get_table_metadata` classmethod calls `cls.get_table_metadata` with the same signature it defines, which for `OdpsBaseEngineSpec` itself results in infinite recursion and a `RecursionError` whenever it is invoked; it should instead delegate to the base implementation on `BaseEngineSpec` and ignore the extra `partition` argument. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Any direct OdpsBaseEngineSpec.get_table_metadata call crashes. - ⚠️ Blocks reuse of OdpsBaseEngineSpec in future subclasses. - ⚠️ Risk for test code importing OdpsBaseEngineSpec directly. ``` </details> ```suggestion # Delegate to the base implementation; the partition argument is currently unused. return super().get_table_metadata(database, table) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import the ODPS engine spec base class from the PR code: `from superset.db_engine_specs.odps import OdpsBaseEngineSpec` (defined at `superset/db_engine_specs/odps.py:44`). 2. In any Python context (tests, shell, or application code), call `OdpsBaseEngineSpec.get_table_metadata(database=None, table=None)`; the arguments are not used before the recursion (`superset/db_engine_specs/odps.py:46-59`). 3. The classmethod body executes `return cls.get_table_metadata(database, table, partition)` which re-invokes the same `OdpsBaseEngineSpec.get_table_metadata` implementation with identical arguments (line 59). 4. This self-call repeats with no termination condition until Python's recursion limit is exceeded, raising a `RecursionError` before any actual metadata lookup can occur. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/db_engine_specs/odps.py **Line:** 59:59 **Comment:** *Logic Error: The `OdpsBaseEngineSpec.get_table_metadata` classmethod calls `cls.get_table_metadata` with the same signature it defines, which for `OdpsBaseEngineSpec` itself results in infinite recursion and a `RecursionError` whenever it is invoked; it should instead delegate to the base implementation on `BaseEngineSpec` and ignore the extra `partition` argument. 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%2F38174&comment_hash=7b2449889ee08f775c86e1cac336f57e1bc90e3455a0ad5b19f7c1462268c3d1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38174&comment_hash=7b2449889ee08f775c86e1cac336f57e1bc90e3455a0ad5b19f7c1462268c3d1&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]
