redash icon indicating copy to clipboard operation
redash copied to clipboard

feature/add support Athena non default catalog

Open h-imaoka opened this issue 2 years ago • 4 comments

What type of PR is this?

  • [x] Refactor
  • [x] Feature
  • [ ] Bug Fix
  • [ ] New Query Runner (Data Source)
  • [ ] New Alert Destination
  • [ ] Other

Description

Athena Federated Query needs non default catalog_name(AwsDataCatalog). This PR supports another athena catalog_name.

1. boto3, botocore version-up

Now, boto3 / botocore freezed very old version. Athena Federated Query GA 12. 2020.

2. get_schema from Athena-api

Current implementation use Glue-api. athena's catalog_name != glue's catalog_name, ( glue's catalog_name = aws accont-id). So, change get_schema from glue to athena's api. There is no performance difference between glue and athena. When compared with about 2000 tables, only a few seconds difference. I also implemented via information_schema.

3. Keep pyathena 1.x

pyathena 2.x supports non default catalog.But we can set catalog name via sql e.g.) select * from [catalog].[database].[table] So, keep pyathena 1.x, I cloud not find the advantage of connecting by specifying the catalog name. (Even in redash 8, we can execute Federated Query by specifying the catalog name in SQL.)

How is this tested?

  • [x] Unit tests (pytest, jest)
  • [ ] E2E Tests (Cypress)
  • [ ] Manually
  • [ ] N/A

Related Tickets & Documents

https://github.com/getredash/redash/pull/5731

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

h-imaoka avatar Apr 24 '22 16:04 h-imaoka

I found out An error occurred (UnrecognizedClientException) when calling the StartQueryExecution operation: The security token included in the request is invalid this error occured after a while...Was it fine for you @h-imaoka @susodapop

yongchand avatar Jul 27 '22 05:07 yongchand

I haven't seen this error. My guess is it sounds like your security credential expired. Have you tried renewing it?

susodapop avatar Jul 27 '22 12:07 susodapop

Sorry, I tried with IAM Role, and feels like something was mingled with my local machine (where I build my docker image). I guess the PR itself is okay. @susodapop

yongchand avatar Jul 27 '22 22:07 yongchand

Please let me know if you're not able to get this working on your box. If there's a change that this pull request breaks existing connections for users we want to know about that before we release it widely.

susodapop avatar Jul 27 '22 22:07 susodapop

@h-imaoka , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts? We should also address Arik's feedback.

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

guidopetri avatar Aug 20 '23 22:08 guidopetri