feat(s3tables-alpha): add support for partition spec, sort order, and table properties#36811
Conversation
packages/@aws-cdk/aws-s3tables-alpha/test/integration/integ.table-with-partition-sort.ts
Show resolved
Hide resolved
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||||||||||||||
|
|
||||||||||||||
ozelalisen
left a comment
There was a problem hiding this comment.
PR Build is failing, could you make CI/CD working so it is passing?
packages/@aws-cdk/aws-s3tables-alpha/test/integration/integ.table-with-partition-sort.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-s3tables-alpha/test/integration/integ.table-with-partition-sort.ts
Outdated
Show resolved
Hide resolved
88c7100 to
c7a2825
Compare
3f304fd to
1948bf0
Compare
ozelalisen
left a comment
There was a problem hiding this comment.
Left some comments regarding design
| /** | ||
| * The partition transform function (e.g., 'identity', 'day', 'hour', 'bucket', 'truncate'). | ||
| */ | ||
| readonly transform: string; |
There was a problem hiding this comment.
Could you make this an enum? Is this values are defined, maybe can do?
export enum PartitionTransform {
IDENTITY = 'identity',
YEAR = 'year',
MONTH = 'month',
DAY = 'day',
HOUR = 'hour',
BUCKET = 'bucket',
TRUNCATE = 'truncate',
}There was a problem hiding this comment.
Updated this to use a PartitionTransform enum.
There was a problem hiding this comment.
could you address this comment? IcebergTransform is an enum-like class so it can be used here instead string
| /** | ||
| * The sort direction. | ||
| */ | ||
| readonly direction: 'asc' | 'desc'; |
There was a problem hiding this comment.
better to define this as enum as well:
export enum SortDirection {
ASC = 'asc',
DESC = 'desc',
}There was a problem hiding this comment.
or could we make this just as boolean, if it has only two fields:
readonly ascending: boolean;There was a problem hiding this comment.
Updated this to use a SortDirection enum instead of string literals.
I kept this as an enum rather than a boolean so the API stays explicit and matches the underlying Iceberg/CFN value shape.
| // Use addPropertyOverride for properties not yet in L1 types | ||
| // Override schema to include Id field |
There was a problem hiding this comment.
Is this still valid, as now L1 resources are published, do you need to still override?
There was a problem hiding this comment.
If not please update all overrides below to use L1 values
There was a problem hiding this comment.
Removed the overrides.
| /** | ||
| * The sort direction. | ||
| */ | ||
| readonly direction: 'asc' | 'desc'; | ||
|
|
||
| /** | ||
| * The null ordering. | ||
| */ | ||
| readonly nullOrder: 'nulls-first' | 'nulls-last'; |
There was a problem hiding this comment.
same comments apply here as above
| /** | ||
| * Test for table with partition spec, sort order, and table properties | ||
| */ | ||
| class TableWithPartitionSortStack extends core.Stack { |
There was a problem hiding this comment.
extending core.Stack is not recommend pattern, use following pattern
import { App, Stack } from 'aws-cdk-lib'; // import this
class TestStack extends Stack { // update here
}
const app = new App(); // Need changes below There was a problem hiding this comment.
Updated this test to use App and Stack from aws-cdk-lib directly.
| } | ||
| } | ||
|
|
||
| const app = new core.App(); |
There was a problem hiding this comment.
Updated this test to use App and Stack from aws-cdk-lib directly, consistent with the other integration test.
packages/@aws-cdk/aws-s3tables-alpha/test/integration/integ.table-with-partition-sort.ts
Show resolved
Hide resolved
…es, and update docs
… interface property ordering, update transform docs
… ordering, add minimal icebergMetadata example, update integ test bucket name
dd5c4db to
f0fa696
Compare
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 5 hours 3 minutes 14 seconds in the queue, including 2 hours 27 minutes 43 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
This PR adds support for new S3 Tables Iceberg features to the L2 construct library, enabling users to configure partition specifications, sort orders, table properties, and schema field IDs when creating tables.
These features are essential for optimizing query performance and data organization in Iceberg tables.
Description of changes
Enhanced Table construct with new Iceberg metadata properties:
idfield toSchemaFieldPropertyfor schema field identificationIcebergPartitionSpecandIcebergPartitionFieldinterfaces for partition configurationIcebergSortOrderandIcebergSortFieldinterfaces for sort order configurationtablePropertiessupport for custom Iceberg table propertiesIcebergMetadataPropertyto include optionalicebergPartitionSpec,icebergSortOrder, andtablePropertiesDocumentation:
IcebergPartitionField,IcebergPartitionSpec, andIcebergSortOrderDescription of how you validated changes
integ.table-with-partition-sort.tsto validate partition spec, sort order, and table propertiesChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license