fmorillo7694 commented on PR #206: URL: https://github.com/apache/flink-connector-aws/pull/206#issuecomment-5875293060
@gguptp thanks for the thorough second pass — all 66 threads are answered inline; here is the shape of the change (5 commits on top of `37f62bc`, head `d2d3973`), plus findings from a full self-review of the diff that I folded in. ### Correctness - **Partition-key reordering corrupted types on read** (your `GlueTableUtils:159` / `GlueFlinkSchemaProperties:112`): fixed at the root by keying every per-column fidelity parameter by **name** and persisting `flink.schema.column-order` — your `serializeSchema`/`restoreSchema` shape. Views record fidelity too; column comments survive. - **Tables created by other engines were unreadable** — `getTable` threw `Unknown table type: EXTERNAL_TABLE` (and `VIRTUAL_VIEW`), i.e. `DESCRIBE`/`SHOW CREATE TABLE` failed on essentially every Athena/crawler/Spark/Hive table. Now `EXTERNAL_TABLE`/`MANAGED_TABLE`/`GOVERNED` → TABLE, `VIRTUAL_VIEW`/`MATERIALIZED_VIEW` → VIEW, and the Hive type strings (`varchar(n)`, `char(n)`, bare `decimal`, `integer`) convert. `uniontype` is rejected with an explanation. - `alterTable` refuses to overwrite a view or change partition keys; unresolved tables get a `CatalogException` instead of a `ClassCastException`; `flink.schema.*` / `flink.original-*` are rejected as user options. - **Found by the real-Glue run:** Glue rejects `CreatePartition` on a table without a location. Partitioned tables of connectors without a URI location (Kinesis, Kafka) now get a synthetic `flink://db/table`; unpartitioned ones keep none, and stream ARNs / bootstrap servers are never written as a location. - Shared base: the `http-client.type` gate never ran (checked after key translation) and the three documented `http-client.*` timeout/connection options were never consumed — both fixed; unused `url-connection-client` dependency dropped. ### API calls Every lookup is now one `GetDatabase` + one `GetTable`/`GetUserDefinedFunction` (was 3× `GetTable` per `SELECT`, 6 calls per function reference incl. built-ins). The case-insensitive conflict checks and the "verify original name" step were unreachable (storage names are always lowercase) and are gone; the lowercase-clash explanation rides as the cause of the `AlreadyExist` exceptions where it can actually fire. SDK paginators everywhere; typed exceptions; `EntityNotFound`/`InvalidInput`/`AccessDenied`/`OperationTimeout` distinguished with messages naming the object; a Glue timeout is no longer reported as "does not exist". Dead methods and 12 unused constants removed; `@Internal`/`@PublicEvolving` on every class. ### Tests (+91 → 225 unit, 10 moto, 5 e2e) `GluePartitionOperatorTest`, `GlueFunctionOperatorTest`, full parameterised type tables both directions, the fidelity suite you asked for (partitioned reorder regression with every type asserted through a real planner type factory, comments, views, foreign `EXTERNAL_TABLE`/`VIRTUAL_VIEW`, alter guards, reserved options), a factory test proving `aws.endpoint` + credentials reach the client (loopback Glue stub asserting `X-Amz-Target` and the SigV4 `Authorization` header), message/cause assertions, cascade-drop verified at the Glue level, per-column types in the moto SQL test, and two e2e additions: a partitioned-table IT and **`GlueCatalogKinesisEndToEndITCase`** — Kinesis table definitions stored in real Glue, a streaming `INSERT … SELECT … WHERE` from the catalog-resolved source to the catalog-resolved sink against a Localstack Kinesis, records read back out of band. One test-infra bug worth calling out: the "Database does not exist" flakes we had been blaming on Glue eventual consistency were `RealGlueCleanupExtension` deleting *another surefire fork's* live database (4 forks share the account). Cleanup is now scoped to names the JVM handed out; the real-Glue run went from 10 flakes to 0. ### Packaging & docs New `flink-sql-catalog-aws-glue` uber jar (same shape as the `flink-sql-connector-*` modules, bundled-deps NOTICE generated from the dependency tree) — I believe this is the NOTICE context @Samrat002 raised earlier. Docs: the type table listed `byte`/`short`/`long` where the converter writes `tinyint`/`smallint`/`bigint`; rewritten with a Schema fidelity section, the real `http-client.*` list and a Limitations entry for foreign tables; module README reduced to a pointer. ### Verification - Fake mode (JDK 17): aws-base 102/102, catalog 225/225 + moto 10/10, shaded jar builds, spotless + checkstyle clean. - **Real AWS Glue (eu-central-1)**: unit suite 225 run / 0 failures / 0 errors / 0 flakes (22 fault-injection skips); e2e 4/4 + the Kinesis-through-catalog IT green against real Glue + Localstack; account left clean. - Fork CI is running on `d2d3973`; apache-side CI will need a committer's approve-and-run again. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
