twalthr commented on code in PR #28152:
URL: https://github.com/apache/flink/pull/28152#discussion_r3266045804
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -5278,7 +5396,10 @@ public static List arrayExcept(List list1, List list2) {
return Arrays.asList(newArray);
} else {
- int posIndex = posInt;
+ // 1-based index.
Review Comment:
Does this also mean a behavioral change for Flink? We should document all
changes of this class in our release notes. Ideally covered with a test.
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/CalcTest.xml:
##########
@@ -115,7 +115,7 @@ LogicalProject(EXPR$0=[$0],
EXPR$1=[CAST($1):VARCHAR(2147483647) CHARACTER SET "
</Resource>
<Resource name="optimized exec plan">
<![CDATA[
-Calc(select=[a AS EXPR$0, CAST(b AS VARCHAR(2147483647)) AS EXPR$1])
Review Comment:
This looks a bit weird. What is the purpose of still sending the values in
this case. Why is this not optimized away? Are the data types of the result
still correct?
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/ValuesTest.xml:
##########
@@ -118,7 +118,8 @@ LogicalProject(a=[$0], b=[$1], c=[$2])
</Resource>
<Resource name="optimized exec plan">
<![CDATA[
-Values(tuples=[[{ 1, 2, 3 }]], values=[a, b, c])
Review Comment:
This also looks fishy. What is the reason for this?
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -563,8 +563,7 @@ private RelDataType decimalOf2(RelDataType type) {
return createSqlType(SqlTypeName.DECIMAL, 38, 0);
case REAL:
return createSqlType(SqlTypeName.DECIMAL, 14, 7);
- case FLOAT:
- return createSqlType(SqlTypeName.DECIMAL, 14, 7);
+ case FLOAT: // sic
Review Comment:
Has this implications on Flink or do we override this in subclasses anyways?
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/rules/logical/FlinkPruneEmptyRulesTest.xml:
##########
@@ -152,7 +152,13 @@ LogicalMinus(all=[false])
</Resource>
<Resource name="optimized rel plan">
<![CDATA[
-LogicalValues(tuples=[[]])
+LogicalMinus(all=[false])
+:- LogicalMinus(all=[false])
+: :- LogicalValues(tuples=[[]])
+: +- LogicalProject(EXPR$0=[20], EXPR$1=[2])
+: +- LogicalValues(tuples=[[{ 20, 2 }]])
++- LogicalProject(EXPR$0=[40], EXPR$1=[4])
+ +- LogicalValues(tuples=[[{ 40, 4 }]])
Review Comment:
The results look very inefficient. Values are pretty common, is there a way
to restore the old behavior?
##########
flink-table/flink-table-planner/src/main/resources/META-INF/services/org.codehaus.commons.compiler.ICompilerFactory:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+org.codehaus.janino.CompilerFactory
Review Comment:
Is it guaranteed that this SPI file is in all created JAR artifacts. E.g.
table-uber-jar
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/rules/logical/subquery/SubQuerySemiJoinTest.scala:
##########
@@ -1596,9 +1596,6 @@ class SubQuerySemiJoinTest extends SubQueryTestBase {
// LogicalFilter lost variablesSet information.
util.verifyRelPlanNotExpected(sqlQuery1, "joinType=[semi]")
-
- val sqlQuery2 = "SELECT MAX(a) FROM x GROUP BY 1 HAVING EXISTS (SELECT 1
FROM y WHERE d < b)"
- util.verifyRelPlanNotExpected(sqlQuery2, "joinType=[semi]")
Review Comment:
I'm fine with this change. Seems pretty niche to me.
--
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]
