-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-35855][table] Upgrade Calcite to 1.36.0 #26827
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
Conversation
SqlNode e; | ||
} | ||
{ | ||
<CONTAINS_SUBSTR> { s = span(); } |
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.
checked with Flink: currently fails on validation
| <PERCENTILE_CONT> | ||
| <PERCENTILE_DISC> |
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.
checked with Flink: currently fail on validation
// Default is not null. | ||
if (Boolean.TRUE.equals(keyType.getNullable())) { | ||
// BEGIN FLINK MODIFICATION | ||
// writer.keyword("NULL"); |
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.
so far we don't have NULL
type
*/ | ||
@SuppressWarnings("UnnecessaryUnboxing") | ||
@Deterministic | ||
public class SqlFunctions { |
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.
There is a bug in Calcite breaking shading
https://issues.apache.org/jira/browse/CALCITE-6393
this is the reason we have this class here (no Flink changes so far)
...link-table-planner/src/main/java/org/apache/calcite/sql/util/ReflectiveSqlOperatorTable.java
Outdated
Show resolved
Hide resolved
@@ -101,14 +101,14 @@ Sink(table=[default_catalog.default_database.sink], fields=[a, EXPR$1, EXPR$2]) | |||
<Resource name="ast"> | |||
<![CDATA[ | |||
LogicalSink(table=[default_catalog.default_database.sink], fields=[a, b, c]) | |||
+- LogicalProject(a=[5:BIGINT], b=[1:BIGINT], c=[1:BIGINT]) | |||
+- LogicalProject(a=[CAST(5:BIGINT):BIGINT], b=[CAST(1:BIGINT):BIGINT], c=[CAST(1:BIGINT):BIGINT]) |
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.
side effect of WITH
functionality.
It only impacts PartitionableSink
cases
<dependency> | ||
<groupId>commons-codec</groupId> | ||
<artifactId>commons-codec</artifactId> | ||
<version>1.15</version> | ||
<optional>${flink.markBundledAsOptional}</optional> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.commons</groupId> | ||
<artifactId>commons-lang3</artifactId> | ||
<version>3.12.0</version> | ||
<optional>${flink.markBundledAsOptional}</optional> | ||
</dependency> |
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.
required to compile SqlFunctions
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 very much @snuyanzin. Looks mostly good to me.
@@ -2511,6 +2529,11 @@ protected void convertFrom( | |||
// ----- FLINK MODIFICATION END ----- | |||
return; | |||
|
|||
case WITH_ITEM_TABLE_REF: |
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 add a test here? Even if it is just error behavior.
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.
Added RecursiveWithTest
currently it fails while planning
...link-table-planner/src/main/java/org/apache/calcite/sql/util/ReflectiveSqlOperatorTable.java
Outdated
Show resolved
Hide resolved
...table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java
Outdated
Show resolved
Hide resolved
+- Values(type=[RecordType(INTEGER ZERO)], tuples=[[{ 0 }]]) | ||
== Optimized Execution Plan == | ||
Sink(table=[default_catalog.default_database.daily_orders], fields=[user, product, amount, dt]) | ||
+- Calc(select=[123 AS user, 'toothpick' AS product, 1 AS amount, '2022-06-12' AS dt]) | ||
+- Calc(select=[CAST(123 AS BIGINT) AS user, CAST('toothpick' AS VARCHAR(2147483647)) AS product, CAST(1 AS INTEGER) AS amount, CAST('2022-06-12' AS VARCHAR(2147483647)) AS dt]) |
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.
Would be great to get a bit more explanation on the root cause of this change.
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 line is the reason
flink/flink-table/flink-table-planner/src/main/java/org/apache/calcite/rex/RexUtil.java
Line 147 in 4835d9d
castExps.add(rexBuilder.makeCast(lhsType, rhsExp, true, false)); |
before that it was
flink/flink-table/flink-table-planner/src/main/java/org/apache/calcite/rex/RexUtil.java
Line 145 in 9fb8a2e
castExps.add(rexBuilder.makeCast(lhsType, rhsExp)); |
after that it takes nullability into account, and here is cast because of different nullability
@@ -114,7 +119,7 @@ public boolean allowAliasUnnestItems() { | |||
|
|||
@Override | |||
public boolean allowNiladicParentheses() { | |||
return false; | |||
return true; |
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.
Allow niladic parentheses
from javadoc
* Whether to allow parentheses to be specified in calls to niladic functions
* and procedures (that is, functions and procedures with no parameters).
*
* <p>For example, {@code CURRENT_DATE} is a niladic system function. In
* standard SQL it must be invoked without parentheses:
*
* <blockquote><code>VALUES CURRENT_DATE</code></blockquote>
*
* <p>If {@code allowNiladicParentheses}, the following syntax is also valid:
seems it matches out case
https://github.com/apache/calcite/blob/3719447e125982aaa47f0a7b2564f94b9900539e/core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java#L366-L390
@@ -77,7 +77,7 @@ GROUP BY cat, hh]]> | |||
LogicalAggregate(group=[{0, 1}], EXPR$2=[SUM($2)], EXPR$3=[COUNT()]) | |||
+- LogicalProject(cat=[$2], hh=[$6], cnt=[$4]) | |||
+- LogicalFilter(condition=[=(SUBSTR(CAST($5):VARCHAR(2147483647) CHARACTER SET "UTF-16LE", 1, 2), $6)]) | |||
+- LogicalProject(a=[$0], b=[$1], cat=[$2], gmt_date=[$3], cnt=[$4], ts=[$5], hh=[SUBSTR(CAST(LOCALTIME()):VARCHAR(2147483647) CHARACTER SET "UTF-16LE" NOT NULL, 1, 2)]) | |||
+- LogicalProject(a=[$0], b=[$1], cat=[$2], gmt_date=[$3], cnt=[$4], ts=[$5], hh=[SUBSTR(CAST(LOCALTIME):VARCHAR(2147483647) CHARACTER SET "UTF-16LE" NOT NULL, 1, 2)]) |
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.
result of enabling allowNiladicParentheses
in FlinkSqlConformance
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.
LGTM. Thanks @snuyanzin!
The PR bumps Calcite to 1.36.0
Brief change log
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation