Flink 1.17: Support alter table column #7628

linyanghao · 2023-05-17T06:38:57Z

Resolves #7312

linyanghao · 2023-05-22T07:09:42Z

For backward-compatibility, I'm currently remaining the old alterTable API as it is. Not sure if it's the right thing to do.
@hililiwei @stevenzwu @chenjunjiedada Could you take a look at this? Thanks!

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java

chenjunjiedada

+1. @stevenzwu you may also be interested in this.

linyanghao · 2023-05-31T08:40:20Z

@chenjunjiedada Thanks for reviewing! I just fixed a bug on modifying the nullability of columns. You might want to check.

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

stevenzwu · 2023-07-11T17:21:43Z

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

+        pendingUpdate.deleteColumn(dropColumn.getColumnName());
+
+      } else if (change instanceof TableChange.AddWatermark) {
+        throw new UnsupportedOperationException("Adding watermark specs is not supported yet. ");


nit: empty space in the end is not necessary. please check all places

this (the extra whitespace in the end) needs to be addressed.

also the error msg doesn't follow the coding style. you can referring to ArrowVectorAccessor for examples. Please review all the error msgs in this class

E.g., it could be sth like Unsupported schema change: adding watermark specs

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java

stevenzwu · 2023-07-11T21:48:53Z

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java

+    if (!schemaChanges.isEmpty()) {
+      UpdateSchema updateSchema = transaction.updateSchema();
+      FlinkAlterTableUtil.applySchemaChanges(updateSchema, schemaChanges);
+      updateSchema.commit();


maybe commit action can be moved inside FlinkAlterTableUtil? then it would be more natural to move applyManageSnapshots into FlinkAlterTableUtil too as mentioned in a comment below.

That's reasonable.

hililiwei · 2023-07-13T11:08:43Z

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

+        throw new UnsupportedOperationException("Modifying watermark specs is not supported yet. ");
+
+      } else if (change instanceof TableChange.DropWatermark) {
+        throw new UnsupportedOperationException("Watermark specs is not supported yet. ");


nit: the previous exception message indicated the action to be taken, such as add, modify, but there is no. Should unify it?

Thanks for catching this!

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

hililiwei · 2023-07-13T11:21:18Z

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

+  }
+
+  @Test
+  public void testAlterTableAddColumn() {


Do we need to add a case to test when the name of the field to be added already exists?

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

linyanghao · 2023-07-24T06:48:43Z

@stevenzwu @hililiwei Thanks for reviewing! I added a few patches. Could you take a look?

linyanghao · 2023-09-04T09:51:19Z

Hi @stevenzwu @hililiwei, do you have any more comments on this?

stevenzwu

@linyanghao this looks very close to me. just left some minor/nit comments

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

linyanghao · 2023-09-12T09:53:35Z

@stevenzwu I addressed the comments. Regarding the warning message, I modified it so that users will only see it when they try altering schema using the old API. Could you take a look? Thanks!

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

stevenzwu · 2023-09-23T05:39:26Z

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

+        pendingUpdate.deleteColumn(dropColumn.getColumnName());
+
+      } else if (change instanceof TableChange.AddWatermark) {
+        throw new UnsupportedOperationException("Adding watermark specs is not supported yet. ");


this (the extra whitespace in the end) needs to be addressed.

stevenzwu · 2023-09-23T05:40:03Z

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/util/FlinkAlterTableUtil.java

+        pendingUpdate.deleteColumn(dropColumn.getColumnName());
+
+      } else if (change instanceof TableChange.AddWatermark) {
+        throw new UnsupportedOperationException("Adding watermark specs is not supported yet. ");


also the error msg doesn't follow the coding style. you can referring to ArrowVectorAccessor for examples. Please review all the error msgs in this class

E.g., it could be sth like Unsupported schema change: adding watermark specs

stevenzwu · 2023-09-23T05:43:20Z

@linyanghao I left a few comments. can you also rebase?

linyanghao · 2023-09-26T09:42:16Z

@stevenzwu Hi, I rebased and addressed the style issues you mentioned. Could you check?

stevenzwu · 2023-09-27T04:51:48Z

thanks @linyanghao for the contribution and @chenjunjiedada and @hililiwei for the review.

@linyanghao can you create a back port PR? you can follow the example here for back port: #8228

linyanghao · 2023-09-27T09:35:02Z

@stevenzwu Sure. Thanks for reviewing! Also thanks @chenjunjiedada and @hililiwei for reviewing!

linyanghao · 2023-09-27T09:36:48Z

@stevenzwu Wait, Flink does not support ALTER TABLE in 1.16 yet. I guess we don't need to backport?

stevenzwu · 2023-09-27T13:52:11Z

if that's the case, yeah no need to back port

github-actions bot added the flink label May 17, 2023

linyanghao force-pushed the flink-alter-schema branch 2 times, most recently from d4d75bc to fe3521b Compare May 22, 2023 06:38

linyanghao marked this pull request as ready for review May 22, 2023 06:39

linyanghao force-pushed the flink-alter-schema branch from fe3521b to 2264787 Compare May 22, 2023 06:47

linyanghao mentioned this pull request May 22, 2023

Integrate Flink's alter table column #7312

Closed

linyanghao changed the title ~~Flink: Support alter table column~~ Flink 1.17: Support alter table column May 22, 2023

chenjunjiedada reviewed May 22, 2023

View reviewed changes

flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java Outdated Show resolved Hide resolved

chenjunjiedada approved these changes May 31, 2023

View reviewed changes

stevenzwu reviewed Jul 11, 2023

View reviewed changes

hililiwei reviewed Jul 13, 2023

View reviewed changes

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java Show resolved Hide resolved

hililiwei reviewed Jul 13, 2023

View reviewed changes

flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java Show resolved Hide resolved

linyanghao requested review from stevenzwu and hililiwei July 24, 2023 09:06

stevenzwu reviewed Sep 11, 2023

View reviewed changes

linyanghao requested a review from stevenzwu September 12, 2023 09:53

stevenzwu reviewed Sep 23, 2023

View reviewed changes

linyanghao and others added 5 commits September 26, 2023 15:21

update

b7bf30e

checkstyle

831b707

log deprecation

3b8dcda

fix: modify nullability

60fff6f

spotless apply

14f5798

linyanghao added 10 commits September 26, 2023 15:21

style

2a23fc3

tests

8b6ae2a

refactors and styles

551064a

tests and fixes

7585fa4

fix doc

70d5c7b

style

adfa4e4

better assertions & add tests

d22b117

move deprecation message

dcc7109

remove unused logger

bbbe086

style

bac300d

linyanghao force-pushed the flink-alter-schema branch from b017a5c to bac300d Compare September 26, 2023 09:23

linyanghao requested a review from stevenzwu September 26, 2023 10:20

stevenzwu approved these changes Sep 27, 2023

View reviewed changes

stevenzwu merged commit 1e52f2e into apache:master Sep 27, 2023
13 checks passed

This was referenced Feb 19, 2024

[Docs, Flink] Iceberg Flink docs do not include support for enhanced DDL support added in #7628 #9755

Closed

Update flink docs with alter column support #9756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flink 1.17: Support alter table column #7628

Flink 1.17: Support alter table column #7628

linyanghao commented May 17, 2023 •

edited

Loading

linyanghao commented May 22, 2023

chenjunjiedada left a comment

linyanghao commented May 31, 2023

stevenzwu Jul 11, 2023

stevenzwu Sep 23, 2023

stevenzwu Sep 23, 2023

stevenzwu Jul 11, 2023

linyanghao Jul 16, 2023

hililiwei Jul 13, 2023

linyanghao Jul 16, 2023

hililiwei Jul 13, 2023

linyanghao commented Jul 24, 2023

linyanghao commented Sep 4, 2023

stevenzwu left a comment

linyanghao commented Sep 12, 2023

stevenzwu Sep 23, 2023

stevenzwu Sep 23, 2023

stevenzwu commented Sep 23, 2023

linyanghao commented Sep 26, 2023

stevenzwu commented Sep 27, 2023 •

edited

Loading

linyanghao commented Sep 27, 2023

linyanghao commented Sep 27, 2023

stevenzwu commented Sep 27, 2023

Flink 1.17: Support alter table column #7628

Flink 1.17: Support alter table column #7628

Conversation

linyanghao commented May 17, 2023 • edited Loading

linyanghao commented May 22, 2023

chenjunjiedada left a comment

Choose a reason for hiding this comment

linyanghao commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linyanghao commented Jul 24, 2023

linyanghao commented Sep 4, 2023

stevenzwu left a comment

Choose a reason for hiding this comment

linyanghao commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzwu commented Sep 23, 2023

linyanghao commented Sep 26, 2023

stevenzwu commented Sep 27, 2023 • edited Loading

linyanghao commented Sep 27, 2023

linyanghao commented Sep 27, 2023

stevenzwu commented Sep 27, 2023

linyanghao commented May 17, 2023 •

edited

Loading

stevenzwu commented Sep 27, 2023 •

edited

Loading