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

Support Arbitrary Catalog IDs on Athena Data Source #7059

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

dtaniwaki
Copy link
Contributor

@dtaniwaki dtaniwaki commented Jul 14, 2024

What type of PR is this?

  • Feature

Description

Currently, a schema list on an Athena data source is only based on the default catalog. If we have an Athena role which is allowed to use glue catalogs of cross accounts, we can still query them by the runner, but no schema of the other catalogs are listed on the UI.

How is this tested?

  • Unit tests (pytest, jest)
  • Manually

I tested the following test cases.

case result
No catalog ID specified The schema list of the default catalog are displayed.
1 catalog ID specified The schema list of the specified catalog are displayed.
2 catalog IDs specified The merged schema list of 2 catalogs are displayed.

And confirmed that I can see expected schema on the UI.

Related Tickets & Documents

It's related to #5682

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

redash

@dtaniwaki dtaniwaki changed the title Catalog ids Support Catalog IDs on Athena Data Source Jul 14, 2024
@dtaniwaki dtaniwaki changed the title Support Catalog IDs on Athena Data Source Support Arbitrary Catalog IDs on Athena Data Source Jul 14, 2024
@dtaniwaki dtaniwaki force-pushed the catalog-ids branch 5 times, most recently from 4541564 to f8de7d4 Compare July 15, 2024 04:06
@justinclift
Copy link
Member

justinclift commented Jul 16, 2024

The concept for this seems fine. I'm not personally sure of better wording for that text string through, as I'm not familiar with Athena myself.

@eradman @lucydodo @wlach @gaecoli Don't suppose any of you are familiar with the Athena data source type? 😄

@lucydodo
Copy link
Member

The text looks good enough as it, but I think it could be made more explicit by changing it to something like this:
Enter Glue Data Catalog IDs, separated by commas (leave blank for default catalog)

@dtaniwaki
Copy link
Contributor Author

I updated the text. Thank you!

@lucydodo
Copy link
Member

LGTM. Thanks :)
@justinclift The final merge decision is left to you.

Copy link
Member

@justinclift justinclift left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for contributing this @dtaniwaki. 😄

@justinclift justinclift merged commit c244e75 into getredash:master Jul 24, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants