korbit-ai[bot] commented on code in PR #36003:
URL: https://github.com/apache/superset/pull/36003#discussion_r2494617593
##########
superset/db_engine_specs/athena.py:
##########
@@ -92,3 +94,39 @@ def _mutate_label(label: str) -> str:
:return: Conditionally mutated label
"""
return label.lower()
+
+ @classmethod
+ def adjust_engine_params(
+ cls,
+ uri: URL,
+ connect_args: dict[str, Any],
+ catalog: str | None = None,
+ schema: str | None = None,
+ ) -> tuple[URL, dict[str, Any]]:
+ """
+ Adjust the SQLAlchemy URI for Athena with a provided schema.
+
+ For AWS Athena the SQLAlchemy URI looks like this:
+
+
awsathena+rest://{aws_access_key_id}:{aws_secret_access_key}@athena.{region_name}.amazonaws.com:443/{schema_name}?s3_staging_dir={s3_staging_dir}&...
+ """
+ if not schema:
+ return uri, connect_args
+
+ uri = uri.set(database=schema)
+ return uri, connect_args
Review Comment:
### Catalog parameter ignored in URI adjustment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The adjust_engine_params method accepts a catalog parameter but completely
ignores it, only using the schema parameter to set the database field in the
URI.
###### Why this matters
AWS Athena supports both catalog and schema concepts, where catalog
represents the data catalog (like AWS Glue) and schema represents the database
within that catalog. Ignoring the catalog parameter means the method cannot
properly handle multi-catalog scenarios, potentially connecting to the wrong
data source.
###### Suggested change ∙ *Feature Preview*
Modify the method to handle both catalog and schema parameters
appropriately. For Athena, if both are provided, they should be combined or the
catalog should be handled through connect_args:
```python
@classmethod
def adjust_engine_params(
cls,
uri: URL,
connect_args: dict[str, Any],
catalog: str | None = None,
schema: str | None = None,
) -> tuple[URL, dict[str, Any]]:
"""
Adjust the SQLAlchemy URI for Athena with a provided schema and catalog.
"""
if catalog:
connect_args = connect_args.copy()
connect_args['catalog_name'] = catalog
if schema:
uri = uri.set(database=schema)
return uri, connect_args
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44c5bf79-9651-48ed-b4a6-9431f25edad2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44c5bf79-9651-48ed-b4a6-9431f25edad2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44c5bf79-9651-48ed-b4a6-9431f25edad2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44c5bf79-9651-48ed-b4a6-9431f25edad2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44c5bf79-9651-48ed-b4a6-9431f25edad2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:09b19ecd-957c-4efa-a947-4d4df1ef6a17 -->
[](09b19ecd-957c-4efa-a947-4d4df1ef6a17)
--
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]
