korbit-ai[bot] commented on code in PR #32556:
URL: https://github.com/apache/superset/pull/32556#discussion_r1986528201
##########
superset/databases/api.py:
##########
@@ -1055,15 +1057,21 @@
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"])
+ is_partitioned_table, partition_fields =
DatabaseDAO.is_odps_partitioned_table(
+ database, table_name
+ )
Review Comment:
I understand why you might believe placing this as a universal logic in a
generic DAO is acceptable. However, the method `is_odps_partitioned_table` is
database-specific (ODPS) and its presence in a generic class like DatabaseDAO
could make the code harder to maintain. This is because it increases the
complexity of the generic class and doesn’t adhere to the Single Responsibility
Principle.
Here's an alternative solution which involves creating a specialized class
for ODPS to handle any particular ODPS logic:
```python
class OdpsDAO(DatabaseDAO):
def is_partitioned_table(self, database, table_name):
# ODPS specific logic
```
In this way, the DatabaseDAO remains generic and the ODPS specific logic is
encapsulated within the OdpsDAO class. This can help keep the design cleaner
and the code more maintainable.
Apart from maintainability, this approach also improves scalability as new
DAOs can be added for other database-specific logic without modifying the
generic class.
Would love to know your thoughts about this.
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
