You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2024/08/09 12:06:16 UTC
[PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
snuyanzin opened a new pull request, #25186:
URL: https://github.com/apache/flink/pull/25186
## What is the purpose of the change
Options from OPTIONS hint should be present in options section of compiled plan, not only in description
## Brief change log
ContextResolvedTable with merged hints should be used for that
## Verifying this
There are 2 tests:
StatementSetImplTest.java
and CompiledPlanITCase.java
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): ( no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
- The serializers: ( no )
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no)
- The S3 file system connector: (yes )
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1712054885
##########
.editorconfig:
##########
@@ -263,6 +263,9 @@ ij_java_variable_annotation_wrap = normal
ij_java_wrap_first_method_in_call_chain = true
# ij_java_wrap_long_lines = false
+[*.out]
+insert_final_newline = false
Review Comment:
this is very annoying thing
by default at the very beginning of this file there is
`insert_final_newline = true`
for all file types
as a result every time it inserts a new line for every file..
Since *.out files are used to compare file content then extra new line symbol might bring difference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711675719
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -417,4 +426,11 @@ private File createSourceSinkTables() throws IOException {
createTestCsvSourceTable("src", DATA, COLUMNS_DEFINITION);
return createTestCsvSinkTable("sink", COLUMNS_DEFINITION);
}
+
+ private String getPreparedToCompareCompiledPlan(final String planAsString) throws IOException {
+ return TableTestUtil.getPrettyJson(
Review Comment:
in case of pretty json it is much faster to find the diff than in case of everything in one line...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711690657
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -80,17 +80,25 @@ protected void setup() throws Exception {
@Test
void testCompilePlanSql() throws IOException {
CompiledPlan compiledPlan =
- tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
+ tableEnv.compilePlanSql(
Review Comment:
added `testSourceTableWithHints()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711536525
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -80,17 +80,25 @@ protected void setup() throws Exception {
@Test
void testCompilePlanSql() throws IOException {
CompiledPlan compiledPlan =
- tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
+ tableEnv.compilePlanSql(
+ "INSERT INTO MySink SELECT * FROM MyTable"
+ // OPTIONS hints here do not play any significant role
+ // we just have to be sure that these options are present in compile
Review Comment:
nit: ^compile^compiled
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/internal/StatementSetImplTest.java:
##########
@@ -65,15 +65,19 @@ void testGetJsonPlan() throws IOException {
tableEnv.executeSql(sinkTableDdl);
StatementSet stmtSet = tableEnv.createStatementSet();
- stmtSet.addInsertSql("INSERT INTO MySink SELECT * FROM MyTable");
+ stmtSet.addInsertSql(
+ "INSERT INTO MySink SELECT * FROM MyTable "
+ // OPTIONS hints here do not play any significant role
+ // we just have to be sure that these options are present in compile plan
Review Comment:
Ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711539500
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/schema/CatalogSourceTable.java:
##########
@@ -119,7 +119,7 @@ public RelNode toRel(ToRelContext toRelContext) {
!schemaTable.isStreamingMode(),
context.getTableConfig(),
relBuilder,
- schemaTable.getContextResolvedTable(),
+ catalogTable,
Review Comment:
Neat. So the resolved table does not include the hints?
Does the output of `computeContextResolvedTable` contain anything else of interest? (AKA, are we going to come back for something like OPTIONS later?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1713253667
##########
flink-table/flink-table-planner/src/test/resources/jsonplan/testGetJsonPlan.out:
##########
@@ -1,105 +1,88 @@
{
- "flinkVersion": "",
Review Comment:
here changes are (to make it behaving same way as `asJsonString`)
1. indents
2. space between `"` and `:`
3. if there are 2 brackets in a row they should be on one line e.g. `}, {` or `[ {`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25186:
URL: https://github.com/apache/flink/pull/25186#issuecomment-2277803246
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e06aa5d0394f41a0ccfe3fc34df3065ef87536be",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e06aa5d0394f41a0ccfe3fc34df3065ef87536be",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e06aa5d0394f41a0ccfe3fc34df3065ef87536be UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25186:
URL: https://github.com/apache/flink/pull/25186#issuecomment-2283234486
Added `[hotfix]` commit making use the fact that `asJsonString` gives pretty json
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1712058342
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -83,14 +83,25 @@ void testCompilePlanSql() throws IOException {
tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
String expected = TableTestUtil.readFromResource("/jsonplan/testGetJsonPlan.out");
assertThat(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(
- compiledPlan.asJsonString()))))
+ getPreparedToCompareCompilePlan(
+ TableTestUtil.getFormattedJson(compiledPlan.asJsonString())))
.isEqualTo(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(expected))));
+ getPreparedToCompareCompilePlan(TableTestUtil.getFormattedJson(expected)));
Review Comment:
ideally here should be just expected however because of difference between `compiledPlan.asJsonString` and `expected` it is like it is
room for improvement within another PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711553638
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -80,17 +80,25 @@ protected void setup() throws Exception {
@Test
void testCompilePlanSql() throws IOException {
CompiledPlan compiledPlan =
- tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
+ tableEnv.compilePlanSql(
+ "INSERT INTO MySink SELECT * FROM MyTable"
+ // OPTIONS hints here do not play any significant role
+ // we just have to be sure that these options are present in compile
+ // plan
+ + "/*+ OPTIONS('bounded'='true', 'sca.parallelism'='2') */");
Review Comment:
```suggestion
+ "/*+ OPTIONS('bounded'='true', 'scan.parallelism'='2') */");
```
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -80,17 +80,25 @@ protected void setup() throws Exception {
@Test
void testCompilePlanSql() throws IOException {
CompiledPlan compiledPlan =
- tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
+ tableEnv.compilePlanSql(
Review Comment:
Don't modify an existing test but add a new one like `testSourceTableWithHints()`
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/internal/StatementSetImplTest.java:
##########
@@ -65,15 +65,19 @@ void testGetJsonPlan() throws IOException {
tableEnv.executeSql(sinkTableDdl);
StatementSet stmtSet = tableEnv.createStatementSet();
- stmtSet.addInsertSql("INSERT INTO MySink SELECT * FROM MyTable");
+ stmtSet.addInsertSql(
Review Comment:
This test is not required. We can drop it.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/schema/CatalogSourceTable.java:
##########
@@ -119,7 +119,7 @@ public RelNode toRel(ToRelContext toRelContext) {
!schemaTable.isStreamingMode(),
context.getTableConfig(),
relBuilder,
- schemaTable.getContextResolvedTable(),
+ catalogTable,
Review Comment:
rename the variable to `contextTableWithHints`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1712058342
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -83,14 +83,25 @@ void testCompilePlanSql() throws IOException {
tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
String expected = TableTestUtil.readFromResource("/jsonplan/testGetJsonPlan.out");
assertThat(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(
- compiledPlan.asJsonString()))))
+ getPreparedToCompareCompilePlan(
+ TableTestUtil.getFormattedJson(compiledPlan.asJsonString())))
.isEqualTo(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(expected))));
+ getPreparedToCompareCompilePlan(TableTestUtil.getFormattedJson(expected)));
Review Comment:
ideally here should not be `getFormattedJson` however because of difference between `compiledPlan.asJsonString` and `expected` it is like it is
room for improvement within another PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25186:
URL: https://github.com/apache/flink/pull/25186#issuecomment-2283199307
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1713253667
##########
flink-table/flink-table-planner/src/test/resources/jsonplan/testGetJsonPlan.out:
##########
@@ -1,105 +1,88 @@
{
- "flinkVersion": "",
Review Comment:
here changes are
1. indents
2. space between `"` and `:`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711563695
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/schema/CatalogSourceTable.java:
##########
@@ -119,7 +119,7 @@ public RelNode toRel(ToRelContext toRelContext) {
!schemaTable.isStreamingMode(),
context.getTableConfig(),
relBuilder,
- schemaTable.getContextResolvedTable(),
+ catalogTable,
Review Comment:
>So the resolved table does not include the hints?
in fact it didn't include hints before line 108 in this class https://github.com/apache/flink/blob/7f0e0e81bac2cc512d0669b25995ca3b13e262f9/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/schema/CatalogSourceTable.java#L108-L111
where then hints (including option hint) are going to be merged into `catalogTable`, however for some reason it was not used here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1713084757
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -83,14 +83,25 @@ void testCompilePlanSql() throws IOException {
tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
String expected = TableTestUtil.readFromResource("/jsonplan/testGetJsonPlan.out");
assertThat(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(
- compiledPlan.asJsonString()))))
+ getPreparedToCompareCompilePlan(
+ TableTestUtil.getFormattedJson(compiledPlan.asJsonString())))
.isEqualTo(
- TableTestUtil.replaceExecNodeId(
- TableTestUtil.replaceFlinkVersion(
- TableTestUtil.getFormattedJson(expected))));
+ getPreparedToCompareCompilePlan(TableTestUtil.getFormattedJson(expected)));
Review Comment:
done in hotfix commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25186:
URL: https://github.com/apache/flink/pull/25186#discussion_r1711690657
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/CompiledPlanITCase.java:
##########
@@ -80,17 +80,25 @@ protected void setup() throws Exception {
@Test
void testCompilePlanSql() throws IOException {
CompiledPlan compiledPlan =
- tableEnv.compilePlanSql("INSERT INTO MySink SELECT * FROM MyTable");
+ tableEnv.compilePlanSql(
Review Comment:
added `testSourceTableWithHints`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on PR #25186:
URL: https://github.com/apache/flink/pull/25186#issuecomment-2278046272
Are there any interactions between settings and options which we need to consider here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin closed pull request #25186: [FLINK-36026][table] Options from `OPTIONS` hint should be present in compile plan
URL: https://github.com/apache/flink/pull/25186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org