-
Notifications
You must be signed in to change notification settings - Fork 670
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
Add get_query_executions generating DataFrames from Athena query executions detail #1676
Conversation
…utions detail (#1) * Added list_query_executions to Athena module * Modified doc string for Athena's list_query_executions * Added get_query_executions for Athena module * Added typing for list_query_executions * Reduced complexity * Added test * Fixed incompatible type raise by mypy for try_it
Fixed E501 from flake8
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
awswrangler/athena/_utils.py
Outdated
query_executions += response["QueryExecutions"] | ||
unprocessed_query_execution += response["UnprocessedQueryExecutionIds"] | ||
|
||
return pd.json_normalize(query_executions), pd.json_normalize(unprocessed_query_execution) |
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.
Instead of two dataframes, perhaps we can consider having a single one with an additional column indicating the state of the query execution?
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.
@jaidisido
I misunderstood the boto3 explanation on UnprocessedQueryExecutionIds
, thus the doc string for get_query_executions
is wrong .UnprocessedQueryExecutionIds
actually contains query ids that the IAM user did not have access to when they called BatchGetQueryExecution
(https://docs.aws.amazon.com/athena/latest/ug/workgroups-troubleshooting.html).
Still, I think that merging two dataframes into one would make it hard for the user to know if there are some query_id that they don't have access to.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Thank you for your contribution
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Feature or Bugfix-
Detail
list_query_executions
to Athena module for fetching list query execution IDs.get_query_executions
to Athena module for returning DataFrames contain query executions detailsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.