Skip to content
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

[Athena] Fix: missing databases (apply pagination) #2503

Merged
merged 6 commits into from
Dec 24, 2018
Merged

[Athena] Fix: missing databases (apply pagination) #2503

merged 6 commits into from
Dec 24, 2018

Conversation

ialeinikov
Copy link
Contributor

@ialeinikov ialeinikov commented May 1, 2018

Closes #2500.

database_paginator = client.get_paginator('get_databases')
table_paginator = client.get_paginator('get_tables')
databases = (database for databases in database_paginator.paginate()
for database in databases['DatabaseList'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something in these 2 lines doesn't look right. are you sure this is what you wanted?

Copy link
Contributor Author

@ialeinikov ialeinikov May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to paginate over both databases and tables.
A database paginator returns a list of lists, in the l#130 I flatten it inside a generator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would suffice to just indent the double for-loops differently to make it more clear?

database_paginator = client.get_paginator('get_databases')
table_paginator = client.get_paginator('get_tables')
databases = (database for databases in database_paginator.paginate()
for database in databases['DatabaseList'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the change into two for-loops. But don't we need to remove lines 130-131 now?

@ghost ghost assigned arikfr Dec 24, 2018
@ghost ghost added the review label Dec 24, 2018
@arikfr arikfr changed the title adding paginator on databases in glue [Athena] Fix: missing databases (apply pagination) Dec 24, 2018
@arikfr arikfr merged commit 2f7cb1b into getredash:master Dec 24, 2018
@ghost ghost removed the review label Dec 24, 2018
@arikfr
Copy link
Member

arikfr commented Dec 24, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants