[FLINK-15591][table] Support create/drop temporary table in Flink SQL #11917

docete · 2020-04-27T08:06:53Z

What is the purpose of the change

This PR supports create/drop temporary table in Flink SQL

Brief change log

e172a38 support parsing TEMPORARY in table definition in sql parser
74191ef support create/drop temporary table in blink planner
487163f support create/drop temporary table in legacy planner

Verifying this change

This change added tests

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, Yarn/Mesos, 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)

…e in blink planner

…egacy planner

flinkbot · 2020-04-27T08:09:11Z

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 487163f (Mon Apr 27 08:09:11 UTC 2020)

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

KurtYoung · 2020-04-27T08:14:44Z

...k-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java

-					dropTableOperation.getTableIdentifier(),
-					dropTableOperation.isIfExists());
+			if (dropTableOperation.isTemporary()) {
+				boolean dropped = catalogManager.dropTemporaryTable(dropTableOperation.getTableIdentifier());


I would suggest to extend catalogManager.dropTemporaryTable to take care of the logic about ifExists, be consistent with catalogManager.dropTable

TableEnvironment#dropTemporaryTable and dropTemporaryView also return boolean which is consistent with CatalogManager#dropTemporaryTable and dropTemporaryView. Shall we refactor these interfaces in another ticket?

Since we are talking about CatalogManager's interface, I think it's make sense to compare the interfaces belong to CatalogManager. It looks a bit strange that some of the 'ignoreIfNotExists' logic is happened inside CatalogManager, but some of them are handled out of CatalogManager. I would suggest to keep it consistent.

OK. I can refactor the CatalogManager first.

KurtYoung · 2020-04-27T08:19:07Z

...ink-table-planner-blink/src/test/scala/org/apache/flink/table/api/TableEnvironmentTest.scala

  }

+  @Test
+  def testExecuteSqlWithCreateDropTemporaryTable(): Unit = {


Testing only the simplest "create + drop" will be insufficient. At least we have to cover some illegal cases, adding some cases when temporary & normal table co-exists.

BTW, will "show tables" command also show temporary tables?

The ITCase testTemporaryTableMaskPermanentTableWithSameName test co-exists cases.

show tables show both temporary and permanent tables

I don't think testTemporaryTableMaskPermanentTableWithSameName is enough either

flinkbot · 2020-04-27T08:44:00Z

CI report:

91669c7 Travis: CANCELED Azure: FAILURE
647f123 UNKNOWN

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

docete · 2020-04-28T09:15:56Z

@KurtYoung added more tests, pls have another look.

… the logic about ignoreIfExists

KurtYoung

LGTM, thanks for the update, I will merge this after tests passed

KurtYoung · 2020-04-29T09:23:06Z

@flinkbot run azure

KurtYoung · 2020-04-29T09:52:15Z

@rmetzger Looks like the ci system didn't capture the latest commit 647f123, and the manually trigger also doesn't work. Do you know any other ways to trigger the test?

KurtYoung · 2020-04-29T13:14:19Z

@docete can you do a rebase and force push to trigger the tests?

rmetzger · 2020-04-29T13:25:01Z

Sorry for these issues Kurt! I asked Chesnay to look into the bot.

rmetzger · 2020-04-29T14:06:22Z

Chesnay has deployed a (hopefully) fixed version of the bot.

KurtYoung · 2020-04-29T14:24:46Z

@rmetzger Thanks!

rmetzger · 2020-04-29T18:46:18Z

@flinkbot run azure

…ners This closes apache#11917

KurtYoung · 2020-04-30T04:23:01Z

run mvn verify locally and passed

docete added 3 commits April 27, 2020 11:54

[FLINK-15591][sql-parser] Support parsing TEMPORARY in table definition

e172a38

[FLINK-15591][table-planner-blink] Support create/drop temporary tabl…

74191ef

…e in blink planner

[FLINK-15591][table-planner] Support create/drop temporary table in l…

487163f

…egacy planner

rmetzger added the review=description? label Apr 27, 2020

KurtYoung requested changes Apr 27, 2020

View reviewed changes

rmetzger added the component=TableSQL/API label Apr 27, 2020

fixup: more tests

0121336

docete added 2 commits April 29, 2020 12:24

[hotfix][table] CatalogManager.dropTemporaryTable should take care of…

91669c7

… the logic about ignoreIfExists

fixup: checkstyle

647f123

KurtYoung approved these changes Apr 29, 2020

View reviewed changes

KurtYoung pushed a commit to KurtYoung/flink that referenced this pull request Apr 30, 2020

[FLINK-15591][table] Support create/drop temporary table in both plan…

91396e6

…ners This closes apache#11917

KurtYoung closed this in dca4a77 Apr 30, 2020

[FLINK-15591][table] Support create/drop temporary table in Flink SQL #11917

[FLINK-15591][table] Support create/drop temporary table in Flink SQL #11917

Uh oh!

Conversation

docete commented Apr 27, 2020

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

Uh oh!

flinkbot commented Apr 27, 2020

Automated Checks

Review Progress

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flinkbot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

docete commented Apr 28, 2020

Uh oh!

KurtYoung left a comment

Choose a reason for hiding this comment

Uh oh!

KurtYoung commented Apr 29, 2020

Uh oh!

KurtYoung commented Apr 29, 2020

Uh oh!

KurtYoung commented Apr 29, 2020

Uh oh!

rmetzger commented Apr 29, 2020

Uh oh!

rmetzger commented Apr 29, 2020

Uh oh!

KurtYoung commented Apr 29, 2020

Uh oh!

rmetzger commented Apr 29, 2020

Uh oh!

KurtYoung commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented Apr 27, 2020 •

edited

Loading