[SQL] Alter Catalog and Table statements#36571
[SQL] Alter Catalog and Table statements#36571ahmedabu98 wants to merge 10 commits intoapache:masterfrom
Conversation
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for ALTER CATALOG and ALTER TABLE statements in Beam SQL, which is a significant feature addition. The changes are extensive, spanning the SQL parser, DDL execution logic, and the Iceberg IO connector. The implementation is well-structured and accompanied by a comprehensive set of tests for the new functionality.
My review has identified a functional bug in the unparse logic for ALTER TABLE, a potential thread-safety issue in the Iceberg catalog configuration, and an opportunity for code simplification by removing duplicated logic. After addressing these points, this pull request will be in excellent shape.
...tensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterTable.java
Show resolved
Hide resolved
...g/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/iceberg/IcebergCatalog.java
Show resolved
Hide resolved
...nsions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterCatalog.java
Show resolved
Hide resolved
|
Reminder, please take a look at this pr: @Abacn |
|
assign to next reviewer |
|
Assigning reviewers: R: @kennknowles for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Reminder, please take a look at this pr: @kennknowles |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @Abacn for label java. Available commands:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces ALTER CATALOG and ALTER TABLE statements to Beam SQL, enhancing DDL capabilities. The changes include new parser rules, DDL node classes, and interface methods to support these operations. Specific implementations are provided for Iceberg catalogs and tables, as well as a test table provider. The new functionality allows modifying catalog properties, and for tables, it supports updating properties, schema (adding/dropping columns), and partition specifications. Comprehensive unit tests have been added to validate the new features, including a specific test for the limitation of not adding required columns to existing Iceberg tables. The code is well-structured and follows good practices for extensibility.
...src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/iceberg/IcebergMetastore.java
Outdated
Show resolved
Hide resolved
|
Reminder, please take a look at this pr: @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
|
Reminder, please take a look at this pr: @chamikaramj |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @Abacn for label java. Available commands:
|
|
Reminder, please take a look at this pr: @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
|
Reminder, please take a look at this pr: @kennknowles |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
|
Reminder, please take a look at this pr: @chamikaramj |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
...src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/iceberg/IcebergMetastore.java
Outdated
Show resolved
Hide resolved
...nsions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterCatalog.java
Show resolved
Hide resolved
...nsions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterCatalog.java
Show resolved
Hide resolved
...tensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlAlterTable.java
Show resolved
Hide resolved
| import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableList; | ||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
|
|
||
| public class SqlAlterCatalog extends SqlAlter implements BeamSqlParser.ExecutableStatement { |
There was a problem hiding this comment.
Just a question - these DDLs are added in the main sql module. Are they now supported by Beam SQL in general, or is just supported for Icebeg Catalog and Tables?
There was a problem hiding this comment.
This class just funnels the properties down to the TableProvider implementation.
Currently, only the InMemory provider and Iceberg providers support Alter operations. Other providers will throw an UnsupportedOperationException
...l/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/test/AlterTestTableOps.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36571 +/- ##
============================================
- Coverage 40.38% 36.33% -4.05%
+ Complexity 3476 1690 -1786
============================================
Files 1226 1066 -160
Lines 188553 167215 -21338
Branches 3607 1227 -2380
============================================
- Hits 76138 60758 -15380
+ Misses 109012 104259 -4753
+ Partials 3403 2198 -1205
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Addresses #35637
Adds the following features:
Modify a catalog's properties
Modify a table's properties, schema, or partition spec
Limitations: