[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax #18236

LadyForest · 2021-12-29T15:13:28Z

What is the purpose of the change

This PR is a foundation of FLINK-25176 that introduces the SQL syntax ALTET TABLE table_identifier [PARTITION partition_spec] COMPACT, which paves the way for coordination with managed table storage. You can find more details at FLIP-188.

Brief changelog

This PR contains two commits

69a521d
- fix typo in UseCatalogOperation
- fix incorrect {@link} in PartitioningSpec (which I've confirmed with @godfreyhe)
- unify the asSummaryString method in AlterTableDropConstraintOperation to use tableIdentifier.asSummaryString because tableIdentifier.toStirng is implicitly call tableIdentifier.asSerializableString
- migrate all tests inSqlToOperationConverterTest to assertJ
980177f
- Extend Parser.tdd and parserImpls.ftl templates for flink-sql-parser to support syntax. Add SqlAlterTableCompact as the representition after parsing.
- Extend SqlToOperationConverter to support operation transformation. The converter imposes a check on the underlying table is a Flink's managed table and throws a ValidationException if it is a non-managed one. The converter also checks the partition spec and throws a ValidationException if the partition_spec is invalid.
- Restrict ALTER TABLE RESET from removing the connector option key.

Verifying this change

This change added tests and can be verified as follows:

Added test in FlinkSqlParserImplTest to verify the syntax parsing
Added test in SqlToOperationConverterTest to verify the operation conversion

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2021-12-29T15:16:06Z

CI report:

980177f Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot · 2021-12-29T15:16:33Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit bac3a83 (Wed Dec 29 15:16:33 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

alexandru-tetelea · 2021-12-29T22:04:39Z

...api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableCompactOperation.java

+
+    public Map<String, String> getPartitionSpec() {
+        return partitionSpec == null
+                ? new LinkedHashMap<>()


Sorry, is it important to return a new empty instance or we can just return java.util.Collections.emptyMap()?

You're right. We'd better return Collections.emptyMap(). It just hadn't hit me yet at that time.

JingsongLi

Thanks @LadyForest for your contribution, left some comments.
Can you separate to two commits? The first one is Fix some error and change tests to junit5, the second one is Introduce ALTER...... In that way, can be more clear.

JingsongLi · 2021-12-30T05:44:49Z

...planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java

+        Catalog catalog =
+                catalogManager
+                        .getCatalog(tableIdentifier.getCatalogName())
+                        .orElseThrow(


orElse(null).
isManagedTable accepts nullable catalog.
We can throws exception to a unified mode in below.

JingsongLi · 2021-12-30T05:50:36Z

...planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java

+                List<String> orderedPartitionKeys = resolvedCatalogTable.getPartitionKeys();
+                int index = 0;
+                String exMsg =
+                        orderedPartitionKeys.isEmpty()


I thought about it, maybe it doesn't have to be ordered, just hit the specified partition is OK.
We can just do something like DynamicSinkUtils.validatePartitioning

JingsongLi · 2021-12-30T05:50:56Z

...planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java

+            }
+            return new AlterTableCompactOperation(tableIdentifier, partitionSpec);
+        }
+        throw new UnsupportedOperationException(


ValidationException?

ValidationException?

I'm fine with that.

…t to use assertJ

LadyForest · 2022-01-04T07:54:28Z

Hi @JingsongLi, I've addressed your comments. Would you mind taking another look?

JingsongLi

Thanks @LadyForest for your update. Looks good to me!

This closes apache#18236

rmetzger added the component=TableSQL/API label Dec 29, 2021

alexandru-tetelea reviewed Dec 29, 2021

View reviewed changes

JingsongLi requested changes Dec 30, 2021

View reviewed changes

LadyForest added 2 commits December 30, 2021 15:41

[FLINK-25176][table] Fix typos and migrate SqlToOperationConverterTes…

69a521d

…t to use assertJ

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax

980177f

LadyForest force-pushed the FLINK-25176 branch from 059fb3d to 980177f Compare December 30, 2021 09:14

LadyForest requested a review from JingsongLi December 30, 2021 12:38

JingsongLi approved these changes Jan 4, 2022

View reviewed changes

JingsongLi closed this in 3849f11 Jan 4, 2022

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax

40b719a

This closes apache#18236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax #18236

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax #18236

Uh oh!

LadyForest commented Dec 29, 2021 •

edited

Loading

Uh oh!

flinkbot commented Dec 29, 2021 •

edited

Loading

Uh oh!

flinkbot commented Dec 29, 2021

Uh oh!

alexandru-tetelea Dec 29, 2021

Uh oh!

LadyForest Dec 30, 2021

Uh oh!

JingsongLi left a comment

Uh oh!

JingsongLi Dec 30, 2021

Uh oh!

JingsongLi Dec 30, 2021

Uh oh!

JingsongLi Dec 30, 2021

Uh oh!

LadyForest Dec 30, 2021

Uh oh!

LadyForest commented Jan 4, 2022

Uh oh!

JingsongLi left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax #18236

[FLINK-25176][table] Introduce "ALTER TABLE ... COMPACT" syntax #18236

Uh oh!

Conversation

LadyForest commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief changelog

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

flinkbot commented Dec 29, 2021

Automated Checks

Review Progress

Uh oh!

alexandru-tetelea Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

LadyForest Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

JingsongLi left a comment

Choose a reason for hiding this comment

Uh oh!

JingsongLi Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

JingsongLi Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

JingsongLi Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

LadyForest Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

LadyForest commented Jan 4, 2022

Uh oh!

JingsongLi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LadyForest commented Dec 29, 2021 •

edited

Loading

flinkbot commented Dec 29, 2021 •

edited

Loading