Antalya 26.6 - Fixed refreshing of glue metadata after ALTER calls - #2272
Antalya 26.6 - Fixed refreshing of glue metadata after ALTER calls#2272subkanthi wants to merge 5 commits into
Conversation
…-schema-after-alter-add-column
CI triage — PR #2272Verdict: 0 failures. CI is not red — it just hasn't produced results yet. The current head (
There are no One-paragraph health check of the changeThe diff is tightly scoped to the reported bug ( Automated triage by @blau-ai — no build/test run locally; verdict is based on GitHub check-runs + the praktika S3 report for |
|
TESTING |
…-schema-after-alter-add-column
…-schema-after-alter-add-column
CI Failures AnalysisVerdict: no PR-caused regressions. Head The PR’s own test Related to this PRNone. Pre-existing / unrelated
Flaky / infra
Recommendations
|
|
Ran https://github.com/Altinity/clickhouse-regression/blob/main/iceberg/tests/iceberg_engine/alter_support.py on Glue catalog, and all passed. LGTM |
There was a problem hiding this comment.
Comments from AI:
AI audit note: This review comment was generated by AI (Cursor Grok 4.6).
Audit update for PR #2272 (Glue DataLakeCatalog schema refresh after ALTER):
Confirmed defects:
Medium: `updateSchema` / `createTable` fail-open on schema conversion, then still commit Glue
Impact: `ALTER`/`CREATE` can succeed while Glue `StorageDescriptor.Columns` stays stale or empty. That is the original bug (`SHOW CREATE` wrong, later `DROP COLUMN` can fail with `NOT_FOUND_COLUMN_IN_BLOCK`). Iceberg metadata on S3 is already written; Glue catalog metadata is left inconsistent.
Anchor: `src/Databases/DataLake/GlueCatalog.cpp` / `GlueCatalog::updateSchema`, `GlueCatalog::createTable`, `updateTableInGlue`
Trigger: Any exception in `icebergSchemaToGlueColumns` / `getCurrentSchemaFromMetadata` (malformed or unexpected Iceberg type JSON). `catch (...)` logs a warning, leaves `columns` empty, then `updateTableInGlue` skips `SetColumns` and still updates `metadata_location`.
Why defect: ClickHouse treats Glue columns as the catalog schema for `SHOW CREATE` and `DatabaseDataLake::tryGetTable`. Swallowing conversion errors and continuing is fail-open on the path this PR claims to fix. REST `updateSchema` fails closed on non-retryable errors.
Fix direction (short): Do not call Glue update/create after conversion failure; throw (or return false for retry). Prefer `metadata` / `parseTableSchemaV2Method` before giving up.
Regression test direction (short): Inject a conversion failure (bad nested type or failpoint) on `ALTER ADD COLUMN` and assert the catalog commit fails, Glue columns unchanged, and `SHOW CREATE` does not report success with a stale schema.
Low: Iceberg metadata schema fallback is only used when Glue columns are empty
Impact: After this PR writes a non-empty `StorageDescriptor`, a stale Glue schema is sticky. `SHOW CREATE` / `tryGetTable` will not re-read Iceberg metadata even though `metadata_location` is current. `SHOW TABLES` still skips empty-column tables (`getTablesForDatabase`), so a failed `createTable` conversion can hide a table that `SHOW CREATE` can still resolve via the new fallback.
Anchor: `src/Databases/DataLake/GlueCatalog.cpp` / `GlueCatalog::tryGetTableMetadata` (columns empty vs non-empty branches), `getTablesForDatabase`
Trigger: Conversion fail-open above, or any Glue row with outdated non-empty columns plus a newer `metadata_location`.
Why defect: Two schema sources with different authority: non-empty Glue SD wins even when it disagrees with Iceberg metadata; listing uses a third rule (skip empty columns).
Fix direction (short): If Glue columns are missing or conversion failed, fail closed, or always derive catalog schema from Iceberg metadata (REST-style) and treat Glue columns as display-only.
Regression test direction (short): After a successful `ALTER`, point Glue columns at the old schema (or empty) and assert `SHOW CREATE` / `SHOW TABLES` / `DROP COLUMN` stay consistent with Iceberg metadata.
Coverage summary:
Scope reviewed: Glue catalog write path (`createTable`, `updateSchema`, `updateMetadata`/`updateTableInGlue`), Glue read path (`tryGetTableMetadata`, `SHOW CREATE`, `tryGetTable`), Iceberg `Mutations` `updateSchema` caller, REST parity, type conversion helpers.
Missed-class probes: 10/12 passed; failed: P2, P9; N/A: P3, P5, P12
Categories failed: protocol fail-open (`catch (...)`), dual-entrypoint schema authority (Glue SD vs Iceberg metadata vs `SHOW TABLES`)
Categories passed: P1 write/`ALTER` happy path (columns now pushed on success), P4 (no new settings), P6 (existing Glue ProfileEvents), P7 (Glue-only helpers), P8 (new integration test is a real reproducer), P10 (`new_schema` `fields` matches converter), P11 (metadata cache keyed by URI; new metadata file after `ALTER`), concurrency/lifetime of `CacheBase` + Glue client
Assumptions/limits: Static review of commit `61c7e4a`; no local Glue/moto run. AWS `UpdateTable` merge-vs-replace of omitted `Columns` was not executed; happy-path `ALTER` + `SHOW CREATE` is supported by the new test and reported Glue regression run.
| if (table_specific_properties.has_value() && !table_specific_properties->iceberg_metadata_file_location.empty()) | ||
| { | ||
| const String & metadata_uri = table_specific_properties->iceberg_metadata_file_location; | ||
| if (!metadata_objects.get(metadata_uri)) |
There was a problem hiding this comment.
I found the same block in GlueCatalog::getActualTimestampType. Makes sense to move code in separate method and reuse in both places.
#2085
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed an issue where Glue
DataLakeCatalogtables could retain stale schema metadata afterALTER TABLEoperations.CI/CD Options
Exclude tests:
Regression jobs to run: