rusackas commented on code in PR #38174:
URL: https://github.com/apache/superset/pull/38174#discussion_r3365193388
##########
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:
Done in 35b775def4. Moved the pyodps import to module top with the
`try/except ImportError` pattern (also pulling in `options` and
`BaseODPSError`), and the function now short-circuits with `if ODPS is None:
return False, []`.
##########
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:
Done in 35b775def4. Threaded `partition` through the base class (H3 below),
so the inline engine-spec import is gone entirely and dispatch goes through
`database.db_engine_spec.get_table_metadata`.
##########
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:
Done in 35b775def4. Hoisted all of the inline `from superset.daos.database
import DatabaseDAO` imports to module top.
##########
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:
Done in 35b775def4. Changed `partition_column` to `tuple[str, ...] | None =
None` so the frozen dataclass is hashable, updated the call site in
`databases/api.py` to `tuple(partition_fields)`, and added a hashability
assertion to the parse tests.
--
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]
