[ahmedabu98]

Full proposal here: https://docs.google.com/document/d/1lGynrfHlyvcmmyzJZ2gkL9zK8KF1ZnfkcImnMmM_ciw/edit?tab=t.0#heading=h.kmw5c7qyihe9

Adds support for the following in Beam SQL:

• Creating a table with a primary key
• Adding a primary key to a table
• Dropping a table's primary key

Each table type should have its own logic to modify the underlying data source's table PK

[github-actions]

📋 Review Summary

This PR cleanly adds DDL and catalog support for PRIMARY KEY ... NOT ENFORCED in CREATE EXTERNAL TABLE and ALTER TABLE statements, along with Iceberg catalog integration for identifier fields. The implementation is well-structured and includes thorough parser and Iceberg tests.

🔍 General Feedback

• When combining schema updates (ADD COLUMNS / DROP COLUMNS) and primary key updates in a single ALTER TABLE statement, SqlAlterTable currently validates the primary key against the pre-alter table schema, which can lead to inconsistent state or unexpected validation failures.
• InMemoryMetaStore.alterTable does not refresh its cached Table metadata when the underlying TableProvider updates the table during an ALTER TABLE operation.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

💬 Inline Findings

• sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterTable.java (line 138): 🟡 Medium: validatePrimaryKey checks table.getSchema(), which reflects the schema before columnsToAdd and columnsToDrop are applied in alterOps.updateSchema(...).

Because the grammar allows combining column changes and primary key changes in a single ALTER TABLE statement (as tested in testUnparseAlter_AllOperations), this leads to two issues:

1. ALTER TABLE t DROP COLUMNS (pk_col) DROP PRIMARY KEY ADD PRIMARY KEY (other_col) NOT ENFORCED (or DROP COLUMNS (id) ADD PRIMARY KEY (id) NOT ENFORCED) will pass validatePrimaryKey because id still exists in table.getSchema() prior to updateSchema, and then fail later inside the storage engine after already committing the column drop.
2. ALTER TABLE t ADD COLUMNS (c1 INT NOT NULL) ADD PRIMARY KEY (c1) NOT ENFORCED will fail validatePrimaryKey because c1 is not yet in table.getSchema().

Consider accounting for columnsToAdd and columnsToDropList when validating the primary key columns (or rejecting combined column and primary key alterations if they are not meant to be supported in one statement).

• sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterTable.java (line 288): 🟢 Nit: getOperandList() includes name, setProps, and resetProps, but omits primaryKeyToAdd (as well as the previously added column/partition lists). Consider adding primaryKeyToAdd if non-null so Calcite AST traversal can inspect all SqlNode operands.

[ahmedabu98]

R: @chamikaramj

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[codecov]

Codecov Report

❌ Patch coverage is 80.45977% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.39%. Comparing base (a8d6afd) to head (696a4df).
⚠️ Report is 109 commits behind head on master.

Files with missing lines	Patch %	Lines
.../sdk/extensions/sql/impl/parser/SqlAlterTable.java	70.96%	4 Missing and 5 partials ⚠️
...ns/sql/meta/provider/iceberg/IcebergMetastore.java	64.28%	1 Missing and 4 partials ⚠️
...dk/extensions/sql/meta/provider/AlterTableOps.java	0.00%	2 Missing ⚠️
...nsions/sql/impl/parser/SqlCreateExternalTable.java	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #40826       +/-   ##
=============================================
- Coverage     80.04%   64.39%   -15.66%     
- Complexity        0     2383     +2383     
=============================================
  Files           490      322      -168     
  Lines         82916    15684    -67232     
  Branches          0     2043     +2043     
=============================================
- Hits          66373    10100    -56273     
+ Misses        16543     4806    -11737     
- Partials          0      778      +778     

Flag	Coverage Δ
java	64.39% <80.45%> (?)
python	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.