twalthr commented on code in PR #28316:
URL: https://github.com/apache/flink/pull/28316#discussion_r3518833989
##########
flink-table/flink-sql-parser/pom.xml:
##########
@@ -68,14 +68,15 @@ under the License.
<version>${calcite.version}</version>
<exclusions>
<!--
- "mvn dependency:tree" as of Calcite 1.40.0:
+ "mvn dependency:tree" as of Calcite 1.41.0:
- [INFO] +-
org.apache.calcite:calcite-core:jar:1.40.0:compile
- [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.40.0:compile
- [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.26.0:compile
+ [INFO] +-
org.apache.calcite:calcite-core:jar:1.41.0:compile
+ [INFO] | +-
org.apache.calcite:calcite-linq4j:jar:1.41.0:compile
+ [INFO] | +-
org.apache.calcite.avatica:avatica-core:jar:1.27.0:compile
[INFO] | +-
org.apiguardian:apiguardian-api:jar:1.1.2:compile
- [INFO] | +-
org.checkerframework:checker-qual:jar:3.43.0:compile
- [INFO] | \-
org.apache.commons:commons-math3:jar:3.6.1:runtime
+ [INFO] | +-
org.checkerframework:checker-qual:jar:3.10.0:compile
+ [INFO] | +-
org.apache.commons:commons-math3:jar:3.6.1:runtime
+ [INFO] | \-
org.jooq:joou-java-6:jar:0.9.4:compile
Review Comment:
What is this dependency used for? Can we exclude it?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/LogicalCorrelateToJoinFromTemporalTableFunctionRule.java:
##########
@@ -336,6 +337,11 @@ public RexNode visitFieldAccess(RexFieldAccess
fieldAccess) {
return rexBuilder.makeInputRef(leftSide, leftIndex);
}
+ @Override
+ public RexNode visitNodeAndFieldIndex(RexNodeAndFieldIndex
nodeAndFieldIndex) {
+ throw new ValidationException("not supported yet");
Review Comment:
ValidationException is for users. This is rather internal.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RemoteCorrelateSplitRule.java:
##########
@@ -131,6 +133,11 @@ public RexNode visitFieldAccess(RexFieldAccess
fieldAccess) {
}
}
+ @Override
+ public RexNode visitNodeAndFieldIndex(RexNodeAndFieldIndex
nodeAndFieldIndex) {
+ throw new ValidationException("not supported yet");
Review Comment:
ValidationException is for users. This is rather internal.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/SplitPythonConditionFromCorrelateRule.java:
##########
@@ -217,6 +219,11 @@ public RexNode visitCall(RexCall call) {
.collect(Collectors.toList()));
}
+ @Override
+ public RexNode visitNodeAndFieldIndex(RexNodeAndFieldIndex
nodeAndFieldIndex) {
+ throw new ValidationException("not supported yet");
Review Comment:
ValidationException is for users. This is rather internal.
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/metadata/FlinkRelMdDistinctRowCountTest.scala:
##########
@@ -146,7 +149,7 @@ class FlinkRelMdDistinctRowCountTest extends
FlinkRelMdHandlerTestBase {
assertEquals(1.0, mq.getDistinctRowCount(logicalProject,
ImmutableBitSet.of(8), expr2))
assertEquals(1.0, mq.getDistinctRowCount(logicalProject,
ImmutableBitSet.of(9), expr2))
assertEquals(1.0, mq.getDistinctRowCount(logicalProject,
ImmutableBitSet.of(10), expr2))
- assertEquals(17.13, mq.getDistinctRowCount(logicalProject,
ImmutableBitSet.of(11), expr2), 1e-2)
+ assertEquals(1.0, mq.getDistinctRowCount(logicalProject,
ImmutableBitSet.of(11), expr2), 1e-2)
Review Comment:
Do you have insights on this one?
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/join/SortMergeJoinTest.xml:
##########
@@ -586,7 +586,7 @@ Calc(select=[d, e, f])
<Resource name="ast">
<![CDATA[
LogicalProject(d=[$3], e=[$4], f=[$5])
-+- LogicalFilter(condition=[=($3, null)])
++- LogicalFilter(condition=[false])
Review Comment:
a classic SQL mistake, not sure if the implementer wanted `WHERE d IS NULL`?
Was the optimized exec plan better when merged for the first time?
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/ValuesTest.xml:
##########
@@ -36,7 +36,7 @@ LogicalProject(a=[$0], b=[$1]), rowType=[RecordType(INTEGER
a, DECIMAL(20, 1) b)
Union(all=[true], union=[EXPR$0, EXPR$1]), rowType=[RecordType(INTEGER EXPR$0,
DECIMAL(20, 1) EXPR$1)]
:- Calc(select=[1 AS EXPR$0, 2.0 AS EXPR$1]), rowType=[RecordType(INTEGER
EXPR$0, DECIMAL(20, 1) EXPR$1)]
: +- Values(tuples=[[{ 0 }]], values=[ZERO]), rowType=[RecordType(INTEGER
ZERO)]
-+- Calc(select=[3 AS EXPR$0, 4.0 AS EXPR$1]), rowType=[RecordType(INTEGER
EXPR$0, DECIMAL(20, 1) EXPR$1)]
++- Calc(select=[3 AS EXPR$0, 4 AS EXPR$1]), rowType=[RecordType(INTEGER
EXPR$0, DECIMAL(20, 1) EXPR$1)]
Review Comment:
Do you know what happened here?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/common/CommonExecMatch.java:
##########
@@ -367,6 +369,12 @@ public Pattern<RowData, RowData> visitCall(RexCall call) {
}
}
+ @Override
+ public Pattern<RowData, RowData> visitNodeAndFieldIndex(
+ RexNodeAndFieldIndex nodeAndFieldIndex) {
+ throw new ValidationException("not supported yet");
Review Comment:
ValidationException is for users. This is rather internal.
```suggestion
throw new UnsupportedOperationException("not supported yet");
```
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/stream/sql/ValuesTest.xml:
##########
@@ -25,9 +25,9 @@ limitations under the License.
LogicalProject(a=[$0], b=[$1]), rowType=[RecordType(INTEGER a, DECIMAL(20, 1)
b)]
+- LogicalProject(a=[$0], b=[$1]), rowType=[RecordType(INTEGER a, DECIMAL(20,
1) b)]
+- LogicalUnion(all=[true]), rowType=[RecordType(INTEGER EXPR$0,
DECIMAL(20, 1) EXPR$1)]
- :- LogicalProject(EXPR$0=[1], EXPR$1=[2.0:DECIMAL(2, 1)]),
rowType=[RecordType(INTEGER EXPR$0, DECIMAL(2, 1) EXPR$1)]
+ :- LogicalProject(EXPR$0=[1], EXPR$1=[2.0:DECIMAL(20, 1)]),
rowType=[RecordType(INTEGER EXPR$0, DECIMAL(20, 1) EXPR$1)]
: +- LogicalValues(tuples=[[{ 0 }]]), rowType=[RecordType(INTEGER ZERO)]
- +- LogicalProject(EXPR$0=[3], EXPR$1=[4:BIGINT]),
rowType=[RecordType(INTEGER EXPR$0, BIGINT EXPR$1)]
+ +- LogicalProject(EXPR$0=[3], EXPR$1=[4:DECIMAL(20, 1)]),
rowType=[RecordType(INTEGER EXPR$0, DECIMAL(20, 1) EXPR$1)]
+- LogicalValues(tuples=[[{ 0 }]]), rowType=[RecordType(INTEGER ZERO)]
Review Comment:
Heavy changes, but looks acceptable.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
