fix(ingest/athena): use schema if db_name is "none" #9271
Conversation
github-actions
bot
added
the
ingestion
maggiehays
added
the
community-contribution
| @@ -383,7 +383,7 @@ def get_database_container_key(self, db_name: str, schema: str) -> DatabaseKey: | |||
| # Based on community feedback, db_name only available if it is explicitly specified in the connection string. | |||
| # If it is not available then we should use schema as db_name | |||
|
|
|||
| if not db_name: | |||
| if not db_name or db_name == "none": | |||
There was a problem hiding this comment.
This seems like a pretty safe change, but I'm a bit confused about when it would be none in lowercase
Is this from str(None).lower() being "none", or something else?
There was a problem hiding this comment.
db_name is "none" when the database parameter in the recipe is not set.
To be honest, I haven't checked where this value comes from and why the original if clause does not work.
But I think your assumption sounds reasonable.
This PR is intended as a quick fix, but I'm open to fixing the underlying cause instead.
There was a problem hiding this comment.
I think we should change this in sql_common to return empty string if the database name is None
Something like this:
def get_db_name(self, inspector: Inspector) -> str:
engine = inspector.engine
if engine and hasattr(engine, "url") and hasattr(engine.url, "database"):
if engine.url.database is None:
return ""
return str(engine.url.database).strip('"').lower()
else:
raise Exception("Unable to get database name from Sqlalchemy inspector")
There was a problem hiding this comment.
This is the file where I think the above should be changed ->
There was a problem hiding this comment.
Sounds good, I'll adapt accordingly
|
indeed 😅 |
When not passing the
databaseparameter for Athena in Datahub0.12.0, all tables and views were collected in the container callednone.This PR uses the schema name if there is no database name provided, e.g., when filtering the assets via
schema_pattern.I kept the previous logical condition, because I'm not sure if this can occur as well.
See error report in slack: https://datahubspace.slack.com/archives/C029A3M079U/p1700121726907619
Checklist