[adapters] Add S3 Tables catalog support to Iceberg input connector.#6608
Conversation
|
@mihaibudiu , can you check the Java changes. |
| /// | ||
| /// Mirrors the table created by the `s3tables` test-setup script: | ||
| /// `id BIGINT NOT NULL`, `name STRING`, `created_at TIMESTAMP`. | ||
| #[derive( |
There was a problem hiding this comment.
I wonder whether we could have a macro generate all this.
| | `s3tables.profile-name` | string | Profile used to access the S3 Tables catalog.| | ||
| | `s3tables.region` | string | Region of the S3 Tables catalog.| | ||
|
|
||
| [*]: These fields are required when the `catalog_type` property is set to `s3tables`. |
There was a problem hiding this comment.
there is only one, but maybe you plan to add more in the future?
| "s3tables.access-key-id": "<AWS_ACCESS_KEY_ID>", | ||
| "s3tables.secret-access-key": "<AWS_SECRET_ACCESS_KEY>", | ||
| "s3tables.region": "us-east-2", | ||
| "s3.access-key-id": "<AWS_ACCESS_KEY_ID>", |
There was a problem hiding this comment.
yep, these are generally two different sets of credentials
| props.insert(S3TABLES_PROP_PROFILE_NAME.to_string(), profile_name.clone()) | ||
| }); | ||
|
|
||
| self.config |
There was a problem hiding this comment.
All these calls look very similar, but I am not sure you can simplify them much. Maybe with a macro.
| /// Specifies the catalog type used to access the Iceberg table. | ||
| /// | ||
| /// Supported options include "rest" and "glue". This property is mutually exclusive with `metadata_location`. | ||
| /// Supported options include "rest", "glue", and "s3tables". This property is mutually |
There was a problem hiding this comment.
Does the documentation really have to specify the values?
That's what the enum is for really.
Or is this for openapi.json?
There was a problem hiding this comment.
I think there is still some issue with how utoipa handles enums. This is more reliable.
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. New catalog integrates cleanly alongside REST/Glue — same config-struct + validate_*_catalog_config + open_table_* pattern, unit tests for the config parsing and cross-catalog rejection, docs and OpenAPI registration all present. Commit hygiene clean (single commit, Signed-off-by, no AI trailers). One small inline observation left below.
| const S3TABLES_PROP_SESSION_TOKEN: &str = "aws_session_token"; | ||
| const S3TABLES_PROP_PROFILE_NAME: &str = "profile_name"; | ||
| const S3TABLES_PROP_REGION_NAME: &str = "region_name"; | ||
|
|
There was a problem hiding this comment.
Soft note: these five keys are duplicated from iceberg-catalog-s3tables's private utils module. If the fork ever renames them (rebase against upstream, refactor, etc.), the catalog will silently fall back to ambient AWS credentials rather than the ones the user configured — a confusing failure mode. Two options worth considering: (1) push a pub re-export upstream in the feldera iceberg-rust fork so we can consume the canonical names, or (2) leave a // TODO(upstream): re-export these and file an issue against the fork. Not blocking.
swanandx
left a comment
There was a problem hiding this comment.
LGTM.
instead of s3tables.session-token, s3tables.profile-name, s3tables.region, ..etc. would it be better if we could specify them like:
"s3tables": { "session-token": .., .. }
or that adds more complexity / unnecessary nested structures?
This would be inconsistent with how we handle other catalogs. The reason we went for a more flat structure is that users aren't good at writing nested JSONs (we've learned this the hard way). It looks like a better design to an engineer, but people consistently misplace attributes... |
The Iceberg input connector supported only REST and Glue catalogs. Add Amazon S3 Tables as a third catalog_type, backed by the iceberg-catalog-s3tables crate from the feldera iceberg-rust fork. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
36286f9 to
a64d9f7
Compare
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
The Iceberg input connector supported only REST and Glue catalogs. Add Amazon S3 Tables as a third catalog_type, backed by the iceberg-catalog-s3tables crate from the feldera iceberg-rust fork.
This doesn't currently have Python integration tests. We need some for this and other Iceberg connector features.
Describe Manual Test Plan
Tested with a single-table pipeline.
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes