mihaibudiu commented on code in PR #4057:
URL: https://github.com/apache/calcite/pull/4057#discussion_r1850838795
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -3148,9 +3148,7 @@ private void
checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT
+ "from t1\n"
+ "where (t1.a, t1.y) in ((1, 2), (3, null), (7369, null), (7499, 30),
(null, 20), (null, 5))";
sql(sql)
- .withRelBuilderConfig(b -> b.withSimplifyValues(false))
- .withRule(CoreRules.PROJECT_VALUES_MERGE,
- CoreRules.UNION_TO_VALUES)
+ .withRule(CoreRules.FILTER_SUB_QUERY_TO_CORRELATE)
Review Comment:
are these tests testing the same thing as the original tests?
if not, perhaps you should add new tests rather than modifying the original
ones
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -6007,6 +6012,26 @@ public List<SqlMonotonicity> getColumnMonotonicities() {
}
}
+ private RexNode getSubqueryRexNode(SqlKind kind,
Review Comment:
I find the name of this function not very easy to understand, perhaps adding
some JavaDoc will help.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1250,6 +1248,31 @@ private void substituteSubQuery(Blackboard bb, SubQuery
subQuery) {
// reference to Q below.
}
+ final RelDataType targetRowType =
+ promoteToRowType(typeFactory,
+ validator().getValidatedNodeType(leftKeyNode), null);
+
+ if (!config.isExpand()) {
+ if (query instanceof SqlNodeList) {
+ // convert
+ // select * from "scott".emp where sal > some (4000, 2000)
+ // to
+ // select * from "scott".emp where sal > some (VALUES (4000), (2000))
+ // The SqlNodeList become a RexSubQuery then optimized by
SubQueryRemoveRule
+ RelNode relNode = convertRowValues(bb, query, (SqlNodeList) query,
false, targetRowType);
Review Comment:
Why are you sure that this is a sequence of literals at this point?
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -3448,13 +3448,13 @@ select * from "scott".emp where empno not in (null,
7782);
!ok
-EnumerableCalc(expr#0..12=[{inputs}], expr#13=[0:BIGINT], expr#14=[=($t8,
$t13)], expr#15=[IS NULL($t12)], expr#16=[>=($t9, $t8)], expr#17=[AND($t15,
$t16)], expr#18=[OR($t14, $t17)], proj#0..7=[{exprs}], $condition=[$t18])
+EnumerableCalc(expr#0..12=[{inputs}], expr#13=[0], expr#14=[=($t8, $t13)],
expr#15=[IS NULL($t12)], expr#16=[>=($t9, $t8)], expr#17=[AND($t15, $t16)],
expr#18=[OR($t14, $t17)], proj#0..7=[{exprs}], $condition=[$t18])
Review Comment:
the disappearance of the cast of 0 may be a hint that the typing is wrong
someplace - either the original plan or the new plan must be wrong.
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -3936,15 +3934,15 @@ select comm, comm in (500, 300, 0) from emp;
!ok
-EnumerableCalc(expr#0..5=[{inputs}], expr#6=[IS NOT NULL($t5)],
expr#7=[0:BIGINT], expr#8=[<>($t1, $t7)], expr#9=[AND($t6, $t8)], expr#10=[IS
NULL($t3)], expr#11=[<($t2, $t1)], expr#12=[OR($t10, $t11)],
expr#13=[null:BOOLEAN], expr#14=[IS NULL($t5)], expr#15=[AND($t12, $t13, $t8,
$t14)], expr#16=[OR($t9, $t15)], COMM=[$t0], EXPR$1=[$t16])
- EnumerableMergeJoin(condition=[=($3, $4)], joinType=[left])
- EnumerableSort(sort0=[$3], dir0=[ASC])
- EnumerableCalc(expr#0..3=[{inputs}], expr#4=[CAST($t1):DECIMAL(10, 2)],
COMM=[$t1], $f0=[$t2], $f1=[$t3], COMM0=[$t4])
+EnumerableCalc(expr#0..6=[{inputs}], expr#7=[0], expr#8=[=($t2, $t7)],
expr#9=[false], expr#10=[CAST($t1):DECIMAL(10, 2)], expr#11=[IS NULL($t10)],
expr#12=[null:BOOLEAN], expr#13=[IS NOT NULL($t6)], expr#14=[true],
expr#15=[<($t3, $t2)], expr#16=[CASE($t8, $t9, $t11, $t12, $t13, $t14, $t15,
$t12, $t9)], COMM=[$t1], EXPR$1=[$t16])
Review Comment:
I cannot review these changes, they are too complex.
##########
core/src/test/resources/sql/some.iq:
##########
@@ -847,5 +847,92 @@ where sal > all (select comm from "scott".emp where comm
is not null);
!ok
+# [CALCITE-4758] When SOME sub-query is SqlNodeList, Calcite returns wrong
result
+
+# make sure the SqlNodeList converted to VALUES not OR condition
+
Review Comment:
this empty line makes it less clear that the comment applies to the
threshold setting
--
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]
