[FLINK-25845][table] Introduce EXECUTE PLAN #18648

slinkydeveloper · 2022-02-07T15:04:27Z

What is the purpose of the change

Introduce EXECUTE PLAN statement.

flinkbot · 2022-02-07T15:08:43Z

CI report:

137aeed Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot · 2022-02-07T15:10:09Z

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 88b2fd7 (Mon Feb 07 15:10:09 UTC 2022)

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.

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

slinkydeveloper · 2022-02-10T08:31:57Z

@flinkbot run azure

twalthr

Very nice work @slinkydeveloper. I added some feedback.

flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dml/SqlExecutePlan.java

...e-api-java/src/main/java/org/apache/flink/table/operations/command/ExecutePlanOperation.java

...-planner/src/test/java/org/apache/flink/table/api/internal/TableEnvironmentInternalTest.java

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCompilePlan.java

twalthr · 2022-02-11T12:18:13Z

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCompilePlan.java

+    private SqlNode checkOperand(SqlNode operand) {
+        if (!(operand instanceof RichSqlInsert || operand instanceof SqlStatementSet)) {
+            throw new UnsupportedOperationException(
+                    "SqlCompilePlan supports only RichSqlInsert or SqlStatementSet as operand");


Could a user see this? If yes, remove internal class names and write the exception like: COMPILE PLAN supports only INSERT INTO or STATEMENT SET definitions.

No this happens only in case of a parser bug or a validator bug. The parser should not be able to parse at all a COMPILE (or a COMPILE AND EXECUTE) if the next AST nodes are not one of these two, because of this: https://github.com/apache/flink/pull/18648/files#diff-3aa4700ca748e2278129878cb20655276a6575183ef6a0b631a219106cf96491R1860-R1864

It would probably be better to use the assert statement instead of UnsupportedOperationException, but the reason i used it is for consistency.

...flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dml/SqlCompileAndExecutePlan.java

twalthr · 2022-02-11T12:29:50Z

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

+                }
+            }
+
+            if (!tableConfig.getConfiguration().get(TableConfigOptions.PLAN_FORCE_RECOMPILE)) {


Let's move this into compiledPlan.writeToFile/into the planner? Then the force overwrite is valid everywhere.

So I added a commit where now writeToFile is consistent with this method. But still I wasn't able to unify them, as there is an essential difference here that while in the sql case i don't have an instance of CompiledPlan already built and with the string value in it, while in table i have it. So i had to duplicate the logic in two different places.

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

...-planner/src/test/java/org/apache/flink/table/api/internal/TableEnvironmentInternalTest.java

…test execution Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com> [fixup] Add ExecutePlanOperation, implement and test it Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…ement and test it Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

… AND EXECUTE PLAN parsing Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…tion Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…anITCase Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

… AND EXECUTE Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

twalthr

LGTM, I had only a minor comment. I can address it while merging. Thanks.

twalthr · 2022-02-14T12:10:32Z

docs/README.md


 ```
-mvn -Pgenerate-config-docs install
+mvn -Pgenerate-config-docs install -DskipTests -Dcheckstyle.skip -Drat.skip -Dscalastyle.skip -Denforcer.skip=true -Dspotless.check.skip=true -Dskip.npm=true -DskipITs=true -Dmaven.javadoc.skip=true -Djapicmp.skip=true


this might a bit too specific, who know what things we need to skip in the future. what we can suggest is -DskipTests -Dfast that should be enough for most use cases, no?

yep let's just use -dfast

This closes apache#18648.

slinkydeveloper force-pushed the FLINK-25845 branch from 919b0ba to 88b2fd7 Compare February 7, 2022 15:04

rmetzger added the component=TableSQL/API label Feb 7, 2022

slinkydeveloper force-pushed the FLINK-25845 branch 2 times, most recently from 7fac1d2 to 2e7e7a6 Compare February 9, 2022 16:52

slinkydeveloper marked this pull request as ready for review February 9, 2022 16:53

twalthr requested changes Feb 11, 2022

View reviewed changes

slinkydeveloper added 12 commits February 11, 2022 14:40

[hotfix][sql-parser] Converted tests to JUnit 5 and enabled parallel …

819a398

…test execution Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[FLINK-25845][sql-parser] Add EXECUTE PLAN parsing

b54b16b

Signed-off-by: slinkydeveloper <francescoguard@gmail.com> [fixup] Add ExecutePlanOperation, implement and test it Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup][table-api-java][table-planner] Add ExecutePlanOperation, impl…

73c80f1

…ement and test it Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[FLINK-25845][sql-parser][table-planner] Add COMPILE PLAN and COMPILE…

9811ca3

… AND EXECUTE PLAN parsing Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Add CompilePlanOperation and CompileAndExecutePlanOperation

c1a31ea

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Implement CompilePlanOperation and CompileAndExecutePlanOpera…

b91b781

…tion Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Parser feedback

3a84c0e

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] @internal annotation

9481077

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Move TableEnvironmentInternalTest and rename it to CompiledPl…

dfde80d

…anITCase Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Now CompiledPlan#writeToFile behaves like COMPILE and COMPILE…

3d72529

… AND EXECUTE Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Removed IOException and now using always TableException

beb1693

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Deduped a bit the tests

44ffd07

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

slinkydeveloper force-pushed the FLINK-25845 branch from 2e7e7a6 to 44ffd07 Compare February 11, 2022 14:32

slinkydeveloper added 2 commits February 14, 2022 09:41

[fixup][docs] Generate docs

64e5cbb

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[hotfix][docs] Improve guide for generating config options docs

137aeed

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

twalthr approved these changes Feb 14, 2022

View reviewed changes

twalthr closed this in 1923d70 Feb 14, 2022

slinkydeveloper deleted the FLINK-25845 branch February 14, 2022 14:57

MrWhiteSike pushed a commit to MrWhiteSike/flink that referenced this pull request Mar 3, 2022

[FLINK-25845][table] Add COMPILE PLAN and COMPILE AND EXECUTE PLAN

497da19

This closes apache#18648.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-25845][table] Introduce EXECUTE PLAN #18648

[FLINK-25845][table] Introduce EXECUTE PLAN #18648

slinkydeveloper commented Feb 7, 2022

flinkbot commented Feb 7, 2022 •

edited

flinkbot commented Feb 7, 2022

slinkydeveloper commented Feb 10, 2022

twalthr left a comment

twalthr Feb 11, 2022

slinkydeveloper Feb 11, 2022 •

edited

twalthr Feb 11, 2022

slinkydeveloper Feb 11, 2022

twalthr left a comment

twalthr Feb 14, 2022

slinkydeveloper Feb 14, 2022

[FLINK-25845][table] Introduce EXECUTE PLAN #18648

[FLINK-25845][table] Introduce EXECUTE PLAN #18648

Conversation

slinkydeveloper commented Feb 7, 2022

What is the purpose of the change

flinkbot commented Feb 7, 2022 • edited

CI report:

flinkbot commented Feb 7, 2022

Automated Checks

Review Progress

slinkydeveloper commented Feb 10, 2022

twalthr left a comment

Choose a reason for hiding this comment

twalthr Feb 11, 2022

Choose a reason for hiding this comment

slinkydeveloper Feb 11, 2022 • edited

Choose a reason for hiding this comment

twalthr Feb 11, 2022

Choose a reason for hiding this comment

slinkydeveloper Feb 11, 2022

Choose a reason for hiding this comment

twalthr left a comment

Choose a reason for hiding this comment

twalthr Feb 14, 2022

Choose a reason for hiding this comment

slinkydeveloper Feb 14, 2022

Choose a reason for hiding this comment

flinkbot commented Feb 7, 2022 •

edited

slinkydeveloper Feb 11, 2022 •

edited