feat: Athena Databases and Tables #5237

richardhboyd · 2019-11-28T04:46:22Z

WIP for Athena Resources via AwsCustomResources

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-11-28T04:46:59Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

richardhboyd · 2019-11-28T04:48:30Z

packages/@aws-cdk/aws-athena/lib/datatypes.ts

+  ARRAY = 'ARRAYTODO',
+  /**
+   * < primitive_type, data_type >
+   */
+  MAP = 'MAPTODO',
+  /**
+   * < col_name : data_type [COMMENT col_comment] [, ...] >
+   */
+  STRUCT = 'STRUCTTODO'


I need a better way to represent these data structures inside the schema.

richardhboyd · 2019-11-28T04:49:10Z

packages/@aws-cdk/aws-athena/lib/table.ts

+
+  /**
+   * The name for the Table.
+   * @default none


Do I need '@default' for required properties?

No, it doesn't really make sense to have @default for required properties - feel free to remove these.

richardhboyd · 2019-11-28T04:49:44Z

packages/@aws-cdk/aws-athena/lib/table.ts

+  this.tableName = props.tableName;
+
+  const s3Policy = new PolicyStatement();
+  s3Policy.addActions('s3:*');


Need to scope this down to the read Actions

Also, you can create a PolicyStatement in one expression:

new AwsCustomResource(this, 'CreateAthenaTable', { // ... policyStatements: [ new PolicyStatement({ actions: [...], resources: [...], }), // ... ], });

richardhboyd · 2019-11-28T04:50:34Z

packages/@aws-cdk/aws-athena/lib/table.ts

+      service: 'Athena',
+      action: 'startQueryExecution',
+      parameters: {
+        QueryString: `CREATE EXTERNAL TABLE IF NOT EXISTS ${props.databaseName}.${props.tableName} (${schemaStringBuilder(props.schema)}) LOCATION 's3://${props.queryBucket.bucketName}'/`,


Add the Comments arguments to the query

richardhboyd · 2019-11-28T04:52:16Z

packages/@aws-cdk/aws-athena/lib/table.ts

+      parameters: {
+        QueryString: `CREATE EXTERNAL TABLE IF NOT EXISTS ${props.databaseName}.${props.tableName} (${schemaStringBuilder(props.schema)}) LOCATION 's3://${props.queryBucket.bucketName}'/`,
+        ResultConfiguration: {
+        OutputLocation: `s3://${props.queryBucket.bucketName}/`


I need a better place for this. we really don't care about the output of this query but this field is required.

richardhboyd

You need to add resources for Workgroups, Catalogs, and (stretch-goal) Athena Connectors. Connectors will require more work than the rest of the changes combined.

aws-cdk-automation · 2019-11-28T05:09:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

netroy · 2019-11-28T19:04:43Z

I really like that CDK is finally getting more Athena support. Thanks ❤️.
However, I'd like to point out a few things.

1. Deprecation of Athena's original metastore

Since Glue has been available across all regions, CREATE DATABASE & CREATE TABLE queries on Athena, actually create databases & tables on the Glue Data Catalog (instead of the metastore Athena originally launched with).
It might make sense to create Athena Database & Table classes as wrappers on top of the already existing classes in @aws-cdk/aws-glue, instead of making custom-resources for them.

More info

2. Avro has special schema requirements

For Apache Avro, column definitions alone don't work right now in Athena.
An Avro style schema also needs to be given,

either as Table.StorageDescriptor.SerDeInfo.Parameters if creating the Table on Glue
or, SERDEPROPERTIES in the CREATE EXTERNAL TABLE query, if creating the Table over Athena, or Spectrum.

More info

richardhboyd · 2019-11-28T21:33:41Z

This is great feedback. I was just working on trying to understand how Glue Tables and Athena Tables were related. On Nov 28, 2019, at 2:05 PM, करतोफ्फेलस्क्रिप्ट™ <notifications@github.com> wrote: I really like that CDK is finally getting more Athena support. Thanks ❤️. However, I'd like to point out a few things. 1. Deprecation of Athena's original metastore Since Glue has been available across all regions, CREATE DATABASE & CREATE TABLE queries on Athena, actually create databases & tables on the Glue Data Catalog (instead of the metastore Athena originally launched with). It might make sense to create Athena Database & Table classes as wrappers on top of the already existing classes in @aws-cdk/aws-glue, instead of making custom-resources for them. More info<https://docs.aws.amazon.com/athena/latest/ug/glue-faq.html> 2. Avro has special schema requirements For Apache Avro, column definitions alone don't work right now in Athena. An Avro style schema also needs to be given, * either as Table.StorageDescriptor.SerDeInfo.Parameters if creating the Table on Glue * or, SERDEPROPERTIES in the CREATE EXTERNAL TABLE query, if creating the Table over Athena, or Spectrum. More info<https://docs.aws.amazon.com/athena/latest/ug/avro.html> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#5237?email_source=notifications&email_token=AN4IKX3QHJBN6ZAUELFV7QDQWAI4ZA5CNFSM4JSPH5AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFNJRIY#issuecomment-559585443>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN4IKX3UMFVR3FFSEM37XS3QWAI4ZANCNFSM4JSPH5AA>.

netroy · 2019-11-28T21:50:05Z

@richardhboyd I created this PR earlier today.
It might be relevant in this context.
Perhaps, I can have your feedback 🙂

skinny85 · 2019-11-29T20:31:49Z

packages/@aws-cdk/aws-athena/package.json

@@ -68,14 +68,18 @@
    "pkglint": "1.18.0"
  },
  "dependencies": {
-    "@aws-cdk/core": "1.18.0"


Leave the @aws-cdk/core dependency (this module uses a bunch of things from it, like Construct).

It was just pushed down a line so the dependencies are in alphabetical order

skinny85 · 2019-11-29T20:38:52Z

packages/@aws-cdk/aws-athena/lib/table.ts

+  const tempStrings: string[] = [];
+  tempStrings.concat(Object.keys(schema)
+    .map(key => `'${key}' '${schema[key]}'`));
+  return `(${tempStrings.join(", ")})`;


This is a very surprising way to write this code... concat doesn't modify its receiver, it returns a new array, and also you're calling concat on something you know is empty.

Did you mean something like this?

function schemaStringBuilder(schema: { [key: string]: DataType }): string { return '(' + Object.keys(schema) .map(key => `'${key}' '${schema[key]}'`) .join(', ') + ')'; }

ohhh, that's good to know. I'll make that change.

mergify · 2019-12-16T12:33:34Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:52Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:10Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:51Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:32Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:32Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

netroy · 2020-01-08T21:34:27Z

@richardhboyd can I somehow help move this forward?
If there is an RFC, I could maybe send some PRs.

Cheers

skinny85

@richardhboyd what's the status of this PR?

Marking as "Request changes" to remove it from my to-do list.

mergify · 2020-03-25T15:53:01Z

Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review.

ayenter · 2020-10-23T22:01:05Z

packages/@aws-cdk/aws-athena/lib/database.ts

+
+  const gluePolicy = new PolicyStatement();
+  gluePolicy.addActions('glue:GetDatabase', 'glue:GetTable');
+  athenaPolicy.addResources(`arn:aws:glue:${Aws.REGION}:${Aws.ACCOUNT_ID}:catalog`);


I think this should be gluePolicy.addResources(...

skinny85 · 2021-06-22T16:18:40Z

I'm closing this one due to inactivity, please comment if you want it re-opened.

first pass at Athena API

c5d9b2f

richardhboyd commented Nov 28, 2019

View reviewed changes

SomayaB assigned skinny85 Nov 28, 2019

skinny85 reviewed Nov 29, 2019

View reviewed changes

SomayaB added draft pr/work-in-progress This PR is a draft and needs further work. and removed draft labels Jan 7, 2020

AlexCheema approved these changes Feb 4, 2020

View reviewed changes

skinny85 suggested changes Mar 12, 2020

View reviewed changes

ayenter reviewed Oct 23, 2020

View reviewed changes

skinny85 closed this Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Athena Databases and Tables #5237

feat: Athena Databases and Tables #5237

richardhboyd commented Nov 28, 2019

mergify bot commented Nov 28, 2019

richardhboyd Nov 28, 2019

richardhboyd Nov 28, 2019

skinny85 Nov 29, 2019

richardhboyd Nov 28, 2019

skinny85 Nov 29, 2019

richardhboyd Nov 28, 2019

richardhboyd Nov 28, 2019

richardhboyd left a comment

aws-cdk-automation commented Nov 28, 2019

netroy commented Nov 28, 2019

richardhboyd commented Nov 28, 2019 via email

netroy commented Nov 28, 2019

skinny85 Nov 29, 2019

richardhboyd Nov 29, 2019

skinny85 Nov 29, 2019 •

edited

Loading

richardhboyd Nov 29, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

netroy commented Jan 8, 2020

skinny85 left a comment

mergify bot commented Mar 25, 2020

ayenter Oct 23, 2020

skinny85 commented Jun 22, 2021

feat: Athena Databases and Tables #5237

feat: Athena Databases and Tables #5237

Conversation

richardhboyd commented Nov 28, 2019

WIP for Athena Resources via AwsCustomResources

mergify bot commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardhboyd left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

netroy commented Nov 28, 2019

1. Deprecation of Athena's original metastore

2. Avro has special schema requirements

richardhboyd commented Nov 28, 2019 via email

netroy commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

netroy commented Jan 8, 2020

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 25, 2020

Choose a reason for hiding this comment

skinny85 commented Jun 22, 2021

skinny85 Nov 29, 2019 •

edited

Loading