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 2025/11/02 21:12:11 UTC
[PR] [FLINK-38609][table] Make generation of xml plans deterministic [flink]
snuyanzin opened a new pull request, #27183:
URL: https://github.com/apache/flink/pull/27183
## What is the purpose of the change
The PR is based on [ [FLINK-38608][table] Drop abandoned plans without related tests]( https://github.com/apache/flink/pull/27181)
The main idea here is to enable xml plan generation predictable (in alphabetical order)
The downside here: it will trigger to regenerate existing plans, however it will be done only once (in this PR)
## Brief change log
plans and DiffRepository (mostly synced with Calcite's version)
## Verifying this change
existing tests
## 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: (no)
## Documentation
- Does this pull request introduce a new feature? ( no)
- If yes, how is the feature documented? (not applicable)
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version (https://github.com/apache/calcite/blob/main/testkit/src/main/java/org/apache/calcite/test/DiffRepository.java), also fixed some bugs both here and in Calcite, like e.g. https://github.com/apache/calcite/pull/4612
There are still some differences between Flink and Calcite versions however now much less.
Probably need a separate ask to try to backport Flink's features to Calcite and drop it from Flink repo
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version (https://github.com/apache/calcite/blob/main/testkit/src/main/java/org/apache/calcite/test/DiffRepository.java), also fixed some bugs both here and in Calcite, like e.g. https://github.com/apache/calcite/pull/4612
There are still some differences between Flink and Calcite versions however now much less.
Probably need a separate task to try to backport Flink's features to Calcite and drop it from Flink repo
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2487975516
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/rules/logical/subquery/SubqueryCorrelateVariablesValidationTest.scala:
##########
@@ -155,8 +154,6 @@ class SubqueryCorrelateVariablesValidationTest extends SubQueryTestBase {
|FROM t1
| WHERE t1a = 'test'
""".stripMargin
- assertThatExceptionOfType(classOf[AssertionError])
- .isThrownBy(() => util.verifyRelPlan(sqlQuery))
Review Comment:
Seems since some time it was fixed and this code was continuing throwing exception with a different reason (nothing in xml plans... ). So generated plans for this
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version, also fixed some bugs both here and in Calcite, like e.g. https://github.com/apache/calcite/pull/4612
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version, also fixed some bugs both here and in Calcite, like e.g. https://github.com/apache/calcite/pull/4612
There are still difference between Flink and Calcite version however now much less.
Probably need a separate ask to try to backport Flink's features to Calcite and drop it from Flink repo
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2487971979
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala:
##########
@@ -1225,16 +1225,23 @@ abstract class TableTestUtilBase(test: TableTestBase, isStreamingMode: Boolean)
// check whether the sql equals to the expected if the `relNodes` are translated from sql
assertSqlEqualsOrExpandFunc()
// check ast plan
+ val plans = new util.HashSet[PlanKind](expectedPlans.toSeq.asJava)
Review Comment:
yep, sure, splitted into several commits
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin closed pull request #27183: [FLINK-38609][table] Make generation of xml plans deterministic
URL: https://github.com/apache/flink/pull/27183
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485025177
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/stream/sql/join/IntervalJoinTest.scala:
##########
@@ -107,7 +107,7 @@ class IntervalJoinTest extends TableTestBase {
/** The time conditions should be an And condition * */
@Test
- def testInteravalNotCnfCondition(): Unit = {
+ def testIntervalNotCnfCondition(): Unit = {
Review Comment:
misprint
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485026216
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala:
##########
@@ -1225,16 +1225,23 @@ abstract class TableTestUtilBase(test: TableTestBase, isStreamingMode: Boolean)
// check whether the sql equals to the expected if the `relNodes` are translated from sql
assertSqlEqualsOrExpandFunc()
// check ast plan
+ val plans = new util.HashSet[PlanKind](expectedPlans.toSeq.asJava)
+
if (expectedPlans.contains(PlanKind.AST)) {
+ plans.remove(PlanKind.AST)
assertEqualsOrExpand("ast", astPlan)
}
// check optimized rel plan
if (expectedPlans.contains(PlanKind.OPT_REL)) {
- assertEqualsOrExpand("optimized rel plan", optimizedRelPlan, expand = false)
+ plans.remove(PlanKind.OPT_REL)
+ // if there is something else in expected plans, then use it for xml generation if needed
+ assertEqualsOrExpand("optimized rel plan", optimizedRelPlan, !plans.isEmpty)
Review Comment:
There are some tests requiring both `OPT_REL`, `OPT_REL_WITH_ADVICE`
in order to have both generated in xml, need to put `true` here if both are present
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2487040339
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala:
##########
@@ -1225,16 +1225,23 @@ abstract class TableTestUtilBase(test: TableTestBase, isStreamingMode: Boolean)
// check whether the sql equals to the expected if the `relNodes` are translated from sql
assertSqlEqualsOrExpandFunc()
// check ast plan
+ val plans = new util.HashSet[PlanKind](expectedPlans.toSeq.asJava)
Review Comment:
Can you put the non-xml changes into a separate 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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2489042285
##########
.editorconfig:
##########
@@ -270,6 +270,11 @@ insert_final_newline = false
indent_style = tab
indent_size = 4
+# For verify plan tests
+[*Test.xml]
+indent_style = space
+indent_size = 2
Review Comment:
sync with `DiffRepository` where indentation is in spaces
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version, also fixed some bugs both here and in Calcite https://github.com/apache/calcite/blob/main/testkit/src/main/java/org/apache/calcite/test/DiffRepository.java, like e.g. https://github.com/apache/calcite/pull/4612
There are still some differences between Flink and Calcite versions however now much less.
Probably need a separate ask to try to backport Flink's features to Calcite and drop it from Flink repo
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485024641
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/api/batch/ExplainTest.xml:
##########
@@ -94,7 +94,7 @@ MultipleInput(readOrder=[0,1], members=[\nHashJoin(joinType=[InnerJoin], where=[
]]>
</Resource>
</TestCase>
- <TestCase name="testExplainWithAgg[extended=true]">
+ <TestCase name="testExplainWithAgg[extended=false]">
Review Comment:
Here and all other xml files: were dropped and completely regenerated
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2485023361
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/DiffRepository.java:
##########
@@ -188,18 +197,42 @@ public class DiffRepository {
* share the same diff-repository: if the repository gets loaded once per test case, then only
* one diff is recorded.
*/
- private static final Map<Class<?>, DiffRepository> MAP_CLASS_TO_REPOSITORY = new HashMap<>();
+ private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE =
Review Comment:
Mostly synced with updates from Calcite version, also fixed some bugs both here and in Calcite, like e.g. https://github.com/apache/calcite/pull/4612
There are still some differences between Flink and Calcite versions however now much less.
Probably need a separate ask to try to backport Flink's features to Calcite and drop it from Flink repo
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #27183:
URL: https://github.com/apache/flink/pull/27183#discussion_r2489664061
##########
.editorconfig:
##########
@@ -270,6 +270,11 @@ insert_final_newline = false
indent_style = tab
indent_size = 4
+# For verify plan tests
+[*Test.xml]
+indent_style = space
+indent_size = 2
Review Comment:
Great 👍 Also had this indentation issue when manually updating xmls
--
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-38609][table] Make generation of xml plans deterministic [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #27183:
URL: https://github.com/apache/flink/pull/27183#issuecomment-3478378141
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a84900d0d7b4842f37e7060a89d5e0e8dffc3497",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a84900d0d7b4842f37e7060a89d5e0e8dffc3497",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a84900d0d7b4842f37e7060a89d5e0e8dffc3497 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