korbit-ai[bot] commented on code in PR #34140:
URL: https://github.com/apache/superset/pull/34140#discussion_r2201487321
##########
superset/db_engine_specs/doris.py:
##########
@@ -248,29 +251,37 @@
catalog: Optional[str] = None,
schema: Optional[str] = None,
) -> tuple[URL, dict[str, Any]]:
- if catalog:
- pass
- elif uri.database and "." in uri.database:
- catalog, _ = uri.database.split(".", 1)
+ # get current catalog/schema
+ if not uri.database:
+ current_catalog, current_schema = DEFAULT_CATALOG, DEFAULT_SCHEMA
+ elif "." not in uri.database:
+ current_catalog, current_schema = DEFAULT_CATALOG, uri.database
else:
- catalog = "internal"
+ current_catalog, current_schema = uri.database.split(".", 1)
- # In Apache Doris, each catalog has an information_schema for BI tool
- # compatibility. See: https://github.com/apache/doris/pull/28919
- schema = schema or "information_schema"
- database = ".".join([catalog or "", schema])
+ # and possibly override them
+ database = ".".join([catalog or current_catalog, schema or
current_schema])
uri = uri.set(database=database)
+
return uri, connect_args
@classmethod
- def get_default_catalog(cls, database: Database) -> Optional[str]:
+ def get_default_catalog(cls, database: Database) -> str:
"""
Return the default catalog.
"""
- if database.url_object.database is None:
- return None
-
- return database.url_object.database.split(".")[0]
+ # first check the URI to see if a default catalog is set
+ if "." in database.url_object.database:
+ return database.url_object.database.split(".")[0]
+
+ # if not, iterate over existing catalogs and find the current one
+ with database.get_sqla_engine() as engine:
+ for catalog in engine.execute("SHOW CATALOGS"):
+ if catalog.IsCurrent:
+ return catalog.CatalogName
Review Comment:
### Missing error handling in catalog query <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The SHOW CATALOGS query execution could fail if the connection is not
established properly, leading to unhandled exceptions.
###### Why this matters
If the database connection fails or if the query execution fails, the code
will raise an exception instead of falling back to the default catalog,
potentially causing application crashes.
###### Suggested change ∙ *Feature Preview*
Add try-except block to handle potential database connection or query
execution failures:
```python
try:
with database.get_sqla_engine() as engine:
for catalog in engine.execute("SHOW CATALOGS"):
if catalog.IsCurrent:
return catalog.CatalogName
except Exception as ex:
logger.warning("Failed to fetch current catalog: %s", str(ex))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8c60b8cb-58c9-40b4-b7d5-c1392eba1eee/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8c60b8cb-58c9-40b4-b7d5-c1392eba1eee?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8c60b8cb-58c9-40b4-b7d5-c1392eba1eee?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8c60b8cb-58c9-40b4-b7d5-c1392eba1eee?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8c60b8cb-58c9-40b4-b7d5-c1392eba1eee)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5ca09636-4246-428f-9094-e8eb7f23ff9e -->
[](5ca09636-4246-428f-9094-e8eb7f23ff9e)
##########
superset/db_engine_specs/doris.py:
##########
@@ -248,29 +251,37 @@ def adjust_engine_params(
catalog: Optional[str] = None,
schema: Optional[str] = None,
) -> tuple[URL, dict[str, Any]]:
- if catalog:
- pass
- elif uri.database and "." in uri.database:
- catalog, _ = uri.database.split(".", 1)
+ # get current catalog/schema
+ if not uri.database:
+ current_catalog, current_schema = DEFAULT_CATALOG, DEFAULT_SCHEMA
+ elif "." not in uri.database:
+ current_catalog, current_schema = DEFAULT_CATALOG, uri.database
else:
- catalog = "internal"
+ current_catalog, current_schema = uri.database.split(".", 1)
- # In Apache Doris, each catalog has an information_schema for BI tool
- # compatibility. See: https://github.com/apache/doris/pull/28919
- schema = schema or "information_schema"
- database = ".".join([catalog or "", schema])
+ # and possibly override them
+ database = ".".join([catalog or current_catalog, schema or
current_schema])
uri = uri.set(database=database)
+
return uri, connect_args
@classmethod
- def get_default_catalog(cls, database: Database) -> Optional[str]:
+ def get_default_catalog(cls, database: Database) -> str:
"""
Return the default catalog.
"""
- if database.url_object.database is None:
- return None
-
- return database.url_object.database.split(".")[0]
+ # first check the URI to see if a default catalog is set
+ if "." in database.url_object.database:
+ return database.url_object.database.split(".")[0]
+
+ # if not, iterate over existing catalogs and find the current one
+ with database.get_sqla_engine() as engine:
+ for catalog in engine.execute("SHOW CATALOGS"):
+ if catalog.IsCurrent:
+ return catalog.CatalogName
Review Comment:
### Unnecessary Database Connection for Catalog Lookup <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The get_default_catalog method creates a new database connection and
executes a query just to find the current catalog, even though it's only needed
as a fallback.
###### Why this matters
Creating unnecessary database connections and executing queries when simpler
alternatives exist can impact performance, especially if this method is called
frequently.
###### Suggested change ∙ *Feature Preview*
Consider caching the current catalog value or passing it through from an
existing connection context where possible. The database connection should only
be created if absolutely necessary:
```python
@classmethod
def get_default_catalog(cls, database: Database) -> str:
# first check the URI to see if a default catalog is set
if database.url_object.database and '.' in database.url_object.database:
return database.url_object.database.split('.')[0]
# If we already have an active connection, reuse it
if hasattr(database, '_current_catalog'):
return database._current_catalog
# Only create connection as last resort
with database.get_sqla_engine() as engine:
for catalog in engine.execute('SHOW CATALOGS'):
if catalog.IsCurrent:
database._current_catalog = catalog.CatalogName
return database._current_catalog
return DEFAULT_CATALOG
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a9416d0-5f78-406f-87ed-68ff8f67e245/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a9416d0-5f78-406f-87ed-68ff8f67e245?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a9416d0-5f78-406f-87ed-68ff8f67e245?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a9416d0-5f78-406f-87ed-68ff8f67e245?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a9416d0-5f78-406f-87ed-68ff8f67e245)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:159c51ba-57ee-44be-a3ee-be8c7f8021a3 -->
[](159c51ba-57ee-44be-a3ee-be8c7f8021a3)
--
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]
