-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-35935][table] Fix RTAS not supporting LIMIT #25185
Conversation
02d6a52
to
e2bb410
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyangzhong Thanks for fixing this! The only question from my side is the reason why single parallelism must set for these tests?
QueryOperation query = | ||
new PlannerQueryOperation( | ||
context.toRelRoot(asQuerySqlNode).project(), | ||
context.toRelRoot(validated).project(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
@@ -78,6 +78,29 @@ void testReplaceTableAS() throws Exception { | |||
verifyCatalogTable(expectCatalogTable, getCatalogTable("target")); | |||
} | |||
|
|||
@Test | |||
void testReplaceTableASWithLimit() throws Exception { | |||
env().setParallelism(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why must be single parallelism?
@@ -108,6 +108,27 @@ void testCreateOrReplaceTableAS() throws Exception { | |||
verifyCatalogTable(expectCatalogTable, getCatalogTable("target")); | |||
} | |||
|
|||
@Test | |||
void testCreateOrReplaceTableASWithLimit() throws Exception { | |||
env().setParallelism(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Hi, @lincoln-lil . The reason is: The plan will be optimized to |
@xuyangzhong So at least we can use a sort clause for batch tests to get the stable result? |
@lincoln-lil You're right. I have replaced all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyangzhong LGTM!
This closes apache#25185 (cherry picked from commit 729b8b8)
This closes apache#25185 (cherry picked from commit 729b8b8)
What is the purpose of the change
When using
SqlReplaceTableAsConverter
to convert SqlNode,SqlReplaceTableAsConverter
wrongly used the original queryasQuerySqlNode
. Actually it should use the validated SqlNode.Brief change log
Verifying this change
Many tests (including UT and IT) have been added to verify this pr.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation