-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ingest) Athena: db filter was not applied #4127
fix(ingest) Athena: db filter was not applied #4127
Conversation
@@ -33,10 +33,22 @@ def get_sql_alchemy_url(self): | |||
|
|||
|
|||
class AthenaSource(SQLAlchemySource): | |||
config: AthenaConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this. The config
is already an instance member of SQLAlchemySource, and will get correctly initialized via lines 44->39.
def __init__(self, config, ctx): | ||
super().__init__(config, ctx, "athena") | ||
|
||
@classmethod | ||
def create(cls, config_dict, ctx): | ||
config = AthenaConfig.parse_obj(config_dict) | ||
return cls(config, ctx) | ||
|
||
# It seems like database/schema filter in the connection string does not work and this to work around that | ||
def get_schema_names(self, inspector): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing return type annotation.
def get_schema_names(self, inspector): | ||
schemas = inspector.get_schema_names() | ||
if self.config.database: | ||
for schema in schemas: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
50-53
return [schema in schemas if schema == self.config.database]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Fix for db filter on Athena * Black formatting * Addressing pr comments * Remove unneeded imports
* Fix for db filter on Athena * Black formatting * Addressing pr comments * Remove unneeded imports
* Fix for db filter on Athena * Black formatting * Addressing pr comments * Remove unneeded imports
Checklist