aminghadersohi commented on code in PR #38174:
URL: https://github.com/apache/superset/pull/38174#discussion_r3360780994
##########
superset/daos/database.py:
##########
@@ -249,6 +251,54 @@ def get_datasets(
.all()
)
+ @classmethod
+ def is_odps_partitioned_table(
+ cls, database: Database, table_name: str
+ ) -> tuple[bool, list[str]]:
+ """
+ This function is used to determine and retrieve
+ partition information of the ODPS table.
+ The return values are whether the partition
+ table is partitioned and the names of all partition fields.
+ """
+ if not database:
+ raise ValueError("Database not found")
+ if database.backend != "odps":
+ return False, []
+ try:
+ from odps import ODPS
Review Comment:
BLOCKER — inline import inside function body, and optional-dependency
imports must use module-top `try/except ImportError` (see `bigquery.py` lines
64–71 for the established pattern). Move to module top:
```python
try:
from odps import ODPS # type: ignore[import-not-found]
except ImportError:
ODPS = None # type: ignore[assignment,misc]
```
Then `if ODPS is None: return False, []` at the top of this function. — Rule
6.
##########
superset/databases/api.py:
##########
@@ -1079,15 +1079,32 @@ def table_metadata(self, pk: int) -> FlaskResponse:
parameters = QualifiedTableSchema().load(request.args)
except ValidationError as ex:
raise InvalidPayloadSchemaError(ex) from ex
-
- table = Table(parameters["name"], parameters["schema"],
parameters["catalog"])
+ table_name = str(parameters["name"])
+ table = Table(table_name, parameters["schema"], parameters["catalog"])
try:
security_manager.raise_for_access(database=database, table=table)
except SupersetSecurityException as ex:
# instead of raising 403, raise 404 to hide table existence
raise TableNotFoundException("No such table") from ex
+ try:
+ is_partitioned_table, partition_fields = (
+ DatabaseDAO.is_odps_partitioned_table(database, table_name)
+ )
+ except Exception as ex: # pylint: disable=broad-except
+ logger.warning(
+ "Error determining ODPS partition info for table %s: %s; "
+ "falling back to non-partitioned path",
+ table_name,
+ error_msg_from_exception(ex),
+ )
+ is_partitioned_table, partition_fields = False, []
+ partition = Partition(is_partitioned_table, partition_fields)
+ if is_partitioned_table:
+ from superset.db_engine_specs.odps import OdpsEngineSpec
Review Comment:
BLOCKER — inline import inside function body. No circular import exists:
this file already imports from `superset.db_engine_specs` at module top (line
113). Move to module top. (If H3 below is addressed by threading `partition`
through the base class, this import disappears entirely.) — Rule 6.
##########
superset/databases/api.py:
##########
@@ -1079,15 +1079,32 @@ def table_metadata(self, pk: int) -> FlaskResponse:
parameters = QualifiedTableSchema().load(request.args)
except ValidationError as ex:
raise InvalidPayloadSchemaError(ex) from ex
-
- table = Table(parameters["name"], parameters["schema"],
parameters["catalog"])
+ table_name = str(parameters["name"])
+ table = Table(table_name, parameters["schema"], parameters["catalog"])
try:
security_manager.raise_for_access(database=database, table=table)
except SupersetSecurityException as ex:
# instead of raising 403, raise 404 to hide table existence
raise TableNotFoundException("No such table") from ex
+ try:
+ is_partitioned_table, partition_fields = (
+ DatabaseDAO.is_odps_partitioned_table(database, table_name)
+ )
+ except Exception as ex: # pylint: disable=broad-except
+ logger.warning(
+ "Error determining ODPS partition info for table %s: %s; "
+ "falling back to non-partitioned path",
+ table_name,
+ error_msg_from_exception(ex),
+ )
+ is_partitioned_table, partition_fields = False, []
+ partition = Partition(is_partitioned_table, partition_fields)
+ if is_partitioned_table:
+ from superset.db_engine_specs.odps import OdpsEngineSpec
- payload = database.db_engine_spec.get_table_metadata(database, table)
+ payload = OdpsEngineSpec.get_table_metadata(database, table,
partition)
+ else:
+ payload = database.db_engine_spec.get_table_metadata(database,
table)
Review Comment:
HIGH — ODPS-specific logic is hardcoded in the generic `table_metadata`
endpoint, bypassing `database.db_engine_spec` dispatch. The engine-spec pattern
exists specifically to prevent this. Right fix: add `partition: Partition |
None = None` to `BaseEngineSpec.get_table_metadata`, pass it through the
generic dispatch call, and let `OdpsEngineSpec` handle it internally. Non-ODPS
specs ignore the parameter; ODPS uses it. As-is, every engine needing an extra
parameter adds another `if engine == "X":` branch here.
##########
tests/unit_tests/db_engine_specs/test_odps.py:
##########
@@ -0,0 +1,185 @@
+# 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.
+import logging
+from unittest.mock import MagicMock, patch
+
+import pytest
+from sqlalchemy.dialects import sqlite
+
+from superset.db_engine_specs.odps import OdpsBaseEngineSpec, OdpsEngineSpec
+from superset.sql.parse import Partition, Table
+
+
+def test_odps_base_engine_spec_get_table_metadata_raises() -> None:
+ """OdpsBaseEngineSpec.get_table_metadata must not be called directly."""
+ with pytest.raises(NotImplementedError):
+ OdpsBaseEngineSpec.get_table_metadata(
+ database=MagicMock(),
+ table=Table("my_table", None, None),
+ )
+
+
+def test_odps_engine_spec_select_star_no_partition() -> None:
+ """select_star for a non-partitioned ODPS table produces a plain SELECT
*."""
+ database = MagicMock()
+ database.backend = "odps"
+ database.get_columns.return_value = []
+ database.compile_sqla_query = lambda query, catalog, schema: str(
+ query.compile(dialect=sqlite.dialect())
+ )
+ dialect = sqlite.dialect()
+
+ sql = OdpsEngineSpec.select_star(
+ database=database,
+ table=Table("my_table", None, None),
+ dialect=dialect,
+ limit=100,
+ show_cols=False,
+ indent=False,
+ latest_partition=False,
+ partition=None,
+ )
+
+ assert "SELECT" in sql
+ assert "my_table" in sql
+
+
+def test_odps_engine_spec_select_star_with_partition() -> None:
+ """select_star for a partitioned ODPS table adds a WHERE clause."""
+ database = MagicMock()
+ database.backend = "odps"
+ database.get_columns.return_value = []
+ database.compile_sqla_query = lambda query, catalog, schema: str(
+ query.compile(dialect=sqlite.dialect())
+ )
+ dialect = sqlite.dialect()
+ partition = Partition(is_partitioned_table=True,
partition_column=["month"])
+
+ sql = OdpsEngineSpec.select_star(
+ database=database,
+ table=Table("my_table", None, None),
+ dialect=dialect,
+ limit=100,
+ show_cols=False,
+ indent=False,
+ latest_partition=False,
+ partition=partition,
+ )
+
+ assert "WHERE" in sql
+
+
+def test_is_odps_partitioned_table_non_odps_backend() -> None:
+ """Returns (False, []) immediately for non-ODPS databases; no network call
made."""
+ from superset.daos.database import DatabaseDAO
Review Comment:
BLOCKER — inline import with no circular-import justification. Same pattern
repeats at lines 88, 100, 119, 137, and 167. Hoist all five to module top
alongside the existing imports. — Rule 6.
##########
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, TYPE_CHECKING
+
+from sqlalchemy import select, text
+from sqlalchemy.engine import Dialect
+
+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: Partition | None = 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
+ """
+ raise NotImplementedError
+
+
+class OdpsEngineSpec(BasicParametersMixin, OdpsBaseEngineSpec):
+ engine = "odps"
+ engine_name = "ODPS (MaxCompute)"
+ default_driver = "odps"
+
+ @classmethod
+ def get_table_metadata(
+ cls, database: Any, table: Table, partition: Partition | None = 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,
+ dialect=engine.dialect,
+ 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,
+ dialect: Dialect,
+ limit: int = 100,
+ show_cols: bool = False,
+ indent: bool = True,
+ latest_partition: bool = True,
+ cols: list[ResultSetColumnType] | None = None,
+ partition: Partition | None = 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 dialect: SqlAlchemy Dialect 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, dialect)
+ qry = select(fields).select_from(text(full_table_name))
+ if database.backend == "odps":
+ if (
+ partition is not None
+ and partition.is_partitioned_table
+ and partition.partition_column is not None
+ and len(partition.partition_column) > 0
+ ):
+ partition_str = partition.partition_column[0]
+ partition_str_where = f"CAST({partition_str} AS STRING) LIKE
'%'"
+ qry = qry.where(text(partition_str_where))
Review Comment:
HIGH — `partition_str` (a column name from ODPS table-schema metadata) is
interpolated bare into a `text()` SQL fragment without identifier quoting.
Column names with SQL metacharacters produce invalid SQL or, in an
attacker-controlled schema, could inject SQL. Use the dialect's identifier
preparer:
```python
quoted = dialect.identifier_preparer.quote(partition_str)
partition_str_where = f"CAST({quoted} AS STRING) LIKE '%'"
```
Also: `LIKE '%'` is a no-op filter (matches everything). An inline comment
explaining *why* a match-all predicate is needed here (presumably to satisfy
ODPS's partition-predicate requirement) would save the next reader. — Rule 27.
##########
superset/daos/database.py:
##########
@@ -249,6 +251,54 @@ def get_datasets(
.all()
)
+ @classmethod
+ def is_odps_partitioned_table(
+ cls, database: Database, table_name: str
+ ) -> tuple[bool, list[str]]:
+ """
+ This function is used to determine and retrieve
+ partition information of the ODPS table.
+ The return values are whether the partition
+ table is partitioned and the names of all partition fields.
+ """
+ if not database:
+ raise ValueError("Database not found")
+ if database.backend != "odps":
+ return False, []
+ try:
+ from odps import ODPS
+ except ImportError:
+ logger.warning("pyodps is not installed, cannot check ODPS
partition info")
+ return False, []
+ uri = database.sqlalchemy_uri
+ access_key = database.password
+ pattern = re.compile(
+
r"odps://(?P<username>[^:]+):(?P<password>[^@]+)@(?P<project>[^/]+)/(?:\?"
+ r"endpoint=(?P<endpoint>[^&]+))"
+ )
+ if not uri or not isinstance(uri, str):
+ logger.warning(
+ "Invalid or missing sqlalchemy URI, please provide a correct
URI"
+ )
+ return False, []
+ if match := pattern.match(unquote(uri)):
+ access_id = match.group("username")
+ project = match.group("project")
+ endpoint = match.group("endpoint")
+ odps_client = ODPS(access_id, access_key, project,
endpoint=endpoint)
+ table = odps_client.get_table(table_name)
Review Comment:
MEDIUM — `odps_client.get_table()` makes a synchronous network call with no
explicit timeout. An unreachable or slow ODPS endpoint blocks the entire
Superset HTTP worker. The pyodps `ODPS` constructor supports
`options`/`timeout` configuration; add a configurable timeout here.
##########
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, TYPE_CHECKING
+
+from sqlalchemy import select, text
+from sqlalchemy.engine import Dialect
+
+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: Partition | None = 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
+ """
+ raise NotImplementedError
+
+
+class OdpsEngineSpec(BasicParametersMixin, OdpsBaseEngineSpec):
+ engine = "odps"
+ engine_name = "ODPS (MaxCompute)"
+ default_driver = "odps"
+
+ @classmethod
+ def get_table_metadata(
+ cls, database: Any, table: Table, partition: Partition | None = None
Review Comment:
MEDIUM — `database: Any` degrades the type from `Database` used in the base
class at line 48. `Database` is already imported under `TYPE_CHECKING` (line
39). Replace `Any` with `Database`. — Rule 12.
##########
superset/sql/parse.py:
##########
@@ -325,6 +325,34 @@ def qualify(
)
+@dataclass(eq=True, frozen=True)
+class Partition:
+ """
+ Partition object, with two attribute keys:
+ is_partitioned_table and partition_column,
+ used to provide partition information
+ Here is an example of an object:
+ {"is_partitioned_table": true, "partition_column": ["month", "day"]}
+ """
+
+ is_partitioned_table: bool
+ partition_column: list[str] | None = None
Review Comment:
HIGH — `Partition` is `@dataclass(frozen=True)`, which auto-generates
`__hash__` from all fields. `list[str]` is not hashable, so
`hash(Partition(True, ["col"]))` raises `TypeError: unhashable type: 'list'` at
runtime — confirmed locally. Change to `tuple[str, ...] | None = None` and
update callers (convert with `tuple(partition_fields)` at the call site in
`databases/api.py`).
##########
superset/databases/api.py:
##########
@@ -1079,15 +1079,32 @@ def table_metadata(self, pk: int) -> FlaskResponse:
parameters = QualifiedTableSchema().load(request.args)
except ValidationError as ex:
raise InvalidPayloadSchemaError(ex) from ex
-
- table = Table(parameters["name"], parameters["schema"],
parameters["catalog"])
+ table_name = str(parameters["name"])
+ table = Table(table_name, parameters["schema"], parameters["catalog"])
try:
security_manager.raise_for_access(database=database, table=table)
except SupersetSecurityException as ex:
# instead of raising 403, raise 404 to hide table existence
raise TableNotFoundException("No such table") from ex
+ try:
+ is_partitioned_table, partition_fields = (
+ DatabaseDAO.is_odps_partitioned_table(database, table_name)
+ )
+ except Exception as ex: # pylint: disable=broad-except
Review Comment:
MEDIUM — `except Exception` is overly broad. `ValueError` (raised when
`database` is None) can't happen here since the database was validated above.
For the pyodps network/auth exceptions: either enumerate the pyodps exception
base types, or add a comment documenting why enumeration isn't feasible. Bare
`except Exception` silently swallows unexpected Python errors (`AttributeError`
from a programming mistake, etc.) that should propagate. — Rule 10.
##########
superset/sql/parse.py:
##########
@@ -325,6 +325,34 @@ def qualify(
)
+@dataclass(eq=True, frozen=True)
+class Partition:
+ """
+ Partition object, with two attribute keys:
+ is_partitioned_table and partition_column,
+ used to provide partition information
+ Here is an example of an object:
+ {"is_partitioned_table": true, "partition_column": ["month", "day"]}
Review Comment:
NIT — docstring example uses JSON notation (`true`/`false`). Use Python
syntax: `Partition(is_partitioned_table=True, partition_column=["month",
"day"])`.
##########
tests/unit_tests/db_engine_specs/test_odps.py:
##########
@@ -0,0 +1,185 @@
+# 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.
+import logging
+from unittest.mock import MagicMock, patch
+
+import pytest
+from sqlalchemy.dialects import sqlite
+
+from superset.db_engine_specs.odps import OdpsBaseEngineSpec, OdpsEngineSpec
+from superset.sql.parse import Partition, Table
+
+
+def test_odps_base_engine_spec_get_table_metadata_raises() -> None:
+ """OdpsBaseEngineSpec.get_table_metadata must not be called directly."""
+ with pytest.raises(NotImplementedError):
+ OdpsBaseEngineSpec.get_table_metadata(
+ database=MagicMock(),
+ table=Table("my_table", None, None),
+ )
+
+
+def test_odps_engine_spec_select_star_no_partition() -> None:
+ """select_star for a non-partitioned ODPS table produces a plain SELECT
*."""
+ database = MagicMock()
+ database.backend = "odps"
+ database.get_columns.return_value = []
+ database.compile_sqla_query = lambda query, catalog, schema: str(
+ query.compile(dialect=sqlite.dialect())
+ )
+ dialect = sqlite.dialect()
+
+ sql = OdpsEngineSpec.select_star(
+ database=database,
+ table=Table("my_table", None, None),
+ dialect=dialect,
+ limit=100,
+ show_cols=False,
+ indent=False,
+ latest_partition=False,
+ partition=None,
+ )
+
+ assert "SELECT" in sql
+ assert "my_table" in sql
+
+
+def test_odps_engine_spec_select_star_with_partition() -> None:
+ """select_star for a partitioned ODPS table adds a WHERE clause."""
+ database = MagicMock()
+ database.backend = "odps"
+ database.get_columns.return_value = []
+ database.compile_sqla_query = lambda query, catalog, schema: str(
+ query.compile(dialect=sqlite.dialect())
+ )
+ dialect = sqlite.dialect()
+ partition = Partition(is_partitioned_table=True,
partition_column=["month"])
+
+ sql = OdpsEngineSpec.select_star(
+ database=database,
+ table=Table("my_table", None, None),
+ dialect=dialect,
+ limit=100,
+ show_cols=False,
+ indent=False,
+ latest_partition=False,
+ partition=partition,
+ )
+
+ assert "WHERE" in sql
+
+
+def test_is_odps_partitioned_table_non_odps_backend() -> None:
+ """Returns (False, []) immediately for non-ODPS databases; no network call
made."""
+ from superset.daos.database import DatabaseDAO
+
+ database = MagicMock()
+ database.backend = "postgresql"
+
+ result = DatabaseDAO.is_odps_partitioned_table(database, "some_table")
+
+ assert result == (False, [])
+
+
+def test_is_odps_partitioned_table_missing_pyodps() -> None:
+ """Returns (False, []) with a warning when pyodps is not installed."""
+ from superset.daos.database import DatabaseDAO
+
+ database = MagicMock()
+ database.backend = "odps"
+ database.sqlalchemy_uri = (
+ "odps://mykey:mysecret@myproject/?endpoint=http://service.odps.test";
+ )
+ database.password = "mysecret" # noqa: S105
+
+ with patch.dict("sys.modules", {"odps": None}):
+ result = DatabaseDAO.is_odps_partitioned_table(database, "some_table")
+
+ assert result == (False, [])
+
+
+def test_is_odps_partitioned_table_uri_no_match(
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """Logs a warning and returns (False, []) when the URI doesn't match the
pattern."""
+ from superset.daos.database import DatabaseDAO
+
+ database = MagicMock()
+ database.backend = "odps"
+ database.sqlalchemy_uri = "odps://invalid-uri-format"
+ database.password = "secret" # noqa: S105
+
+ mock_odps_module = MagicMock()
+ with patch.dict("sys.modules", {"odps": mock_odps_module}):
+ with caplog.at_level(logging.WARNING, logger="superset.daos.database"):
+ result = DatabaseDAO.is_odps_partitioned_table(database,
"some_table")
+
+ assert result == (False, [])
+ assert "did not match" in caplog.text
+
+
+def test_is_odps_partitioned_table_partitioned(monkeypatch:
pytest.MonkeyPatch) -> None:
+ """Returns (True, [field_names]) for a partitioned ODPS table."""
+ from superset.daos.database import DatabaseDAO
+
+ database = MagicMock()
+ database.backend = "odps"
+ database.sqlalchemy_uri = (
+ "odps://mykey:mysecret@myproject/?endpoint=http://service.odps.test";
+ )
+ database.password = "mysecret" # noqa: S105
+
+ mock_partition = MagicMock()
+ mock_partition.name = "month"
+ mock_table = MagicMock()
+ mock_table.exist_partition = True
+ mock_table.table_schema.partitions = [mock_partition]
+
+ mock_odps_client = MagicMock()
+ mock_odps_client.get_table.return_value = mock_table
+ mock_odps_class = MagicMock(return_value=mock_odps_client)
+
+ with patch.dict("sys.modules", {"odps": MagicMock(ODPS=mock_odps_class)}):
+ with patch("superset.daos.database.ODPS", mock_odps_class,
create=True):
Review Comment:
MEDIUM — `patch("superset.daos.database.ODPS", ..., create=True)` creates a
module-level attribute that doesn't exist in production (the production code
does `from odps import ODPS` inline, not at module level). The outer
`patch.dict("sys.modules", {"odps": ...})` is already sufficient. The inner
`create=True` patch is misleading and doesn't control what the function reads.
The inconsistency with `test_is_odps_partitioned_table_not_partitioned` (which
correctly omits the inner patch) confirms this. Remove the inner `patch(...)`
block.
--
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]
