-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-35856] [table] Upgrade Calcite version to 1.37.0 #26895
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you for the hard work @michalstutzmann
the PR already looks good
I left some comments
|
||
val sqlQuery2 = "SELECT MAX(a) FROM x GROUP BY 1 HAVING EXISTS (SELECT 1 FROM y WHERE d < b)" | ||
util.verifyRelPlanNotExpected(sqlQuery2, "joinType=[semi]") | ||
util.verifyRelPlanNotExpected(sqlQuery, "joinType=[semi]") |
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.
what is the reason for such change in tests
flink-table/pom.xml
Outdated
<calcite.version>1.36.0</calcite.version> | ||
<!-- Calcite 1.36.0 depends on 3.1.9, | ||
<calcite.version>1.37.0</calcite.version> | ||
<!-- Calcite 1.37.0 depends on 3.1.9, |
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.
this comment is no longer true
Calcite 1.37 depends on janino 3.1.10
https://central.sonatype.com/artifact/org.apache.calcite/calcite-core/1.37.0/dependencies
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.
not sure, may be the version bump could be done separately if it doesn't block
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.
Have corrected the comment. 1.37 depends already on 3.1.11 which is causing the tests to fail but with 3.1.10 it seems to work. I'll have another look. If bumping to 3.1.11 is a lot of work (lot of changes) I agree it's probably better to do it separately.
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.
Update: trying out building with 3.1.11.
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.
Janino 3.1.11 has changed the way the compiler factory is loaded - has switched to Java's ServiceLoader.
The e2e tests were failing due class loading issues - the META-INF/services from Janino's jar could not be found by ServiceLoader
. Adding the entry to table planner's jar resolves the issue. This is the simplest solution I've found so far. Have upgraded Janino to the latest patch version 3.1.12.
Calc(select=[1 AS a, b, c]) | ||
+- Values(tuples=[[{ 1, 2, 3 }]]) |
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.
I wonder why do we start having extra calc here and for other tests?
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.
It's caused by CALCITE-6044. We could revert to the 1.36 behaviour here.
<TIME> { s = span(); } <WITH> ( <LOCAL> { local = true; } )? <TIME> <ZONE> p = SimpleStringLiteral() { | ||
return SqlLiteral.createUnknown("TIME WITH " + (local ? "LOCAL " : "") + "TIME ZONE", p, s.end(this)); | ||
} | ||
| | ||
LOOKAHEAD(2) | ||
<TIMESTAMP> { s = span(); } <WITH> ( <LOCAL> { local = true; } )? <TIME> <ZONE> p = SimpleStringLiteral() { | ||
return SqlLiteral.createUnknown("TIMESTAMP WITH " + (local ? "LOCAL " : "") + "TIME ZONE", p, s.end(this)); |
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.
is it now also supported for Flink?
/** | ||
* Parses a lambda expression. | ||
*/ | ||
SqlNode LambdaExpression() : |
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.
can we have a dummy test showing that it fails somewhere on validation rather than in runtime?
@snuyanzin @michalstutzmann I see changes in the tests around the SQL and am worried whether this could mean there is a backwards compatibility issue in some way. For example would restoring savepoints across the Calcite version change, work? Is there a test for this? I assume a more efficient optimization at a higher Calcite level does not cause a compatibility issue - though the explain output may change. It would be great if you could reassure me these worries are unfounded. |
@michalstutzmann I see you have thumbs up this question I am not sure what you mean by this. I see the docs say |
@davidradl I will check all those things soon and come back to you. Currently on the Janino upgrade and failing tests there (see earlier comment). |
|
if a change (not only Calcite upgrade) leads to change (not only description change) in CompiledPlan store/restore tests then it is most probably a breaking change. If a change leads to change in optimized plan tests then it is at least questionable... Of course our tests do not cover all the cases and could miss something |
What is the purpose of the change
Upgrades Calcite to version 1.37.0.
This PR covers only Calcite upgrade without the implementation of lambda expressions in SQL (CALCITE-3679) on the Flink side.
Brief change log
Verifying this change
Existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation