libenchao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1225800336
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -6823,6 +6823,50 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule
rule) {
.checkUnchanged();
}
+ /** Test case for CALCITE-5683 for two level nested decorrelate with
standard program
+ * failing during the decorrelation phase. */
+ @Test void testTwoLevelDecorrelate() {
+ final String sql = "SELECT d1.name, d1.deptno + "
+ + " ( SELECT e1.empno "
+ + " FROM emp e1 "
+ + " WHERE d1.deptno = e1.deptno and "
+ + " e1.sal = (SELECT max(sal) "
+ + " FROM emp e2 "
+ + " WHERE e1.sal = e2.sal and"
+ + " e1.deptno = e2.deptno and"
+ + " d1.deptno < e2.deptno))"
+ + " FROM dept d1";
+
+ sql(sql)
+ .withExpand(false)
+ .withLateDecorrelate(true)
+ .withSubQueryRules()
+ .withTrim(true)
+ .withRule()
+ .checkUnchanged();
+ }
+
+ /** Test case for CALCITE-5683 for two level nested decorrelate with
standard program
+ * failing during the decorrelation phase. */
+ @Test void testTwoLevelDecorrelateSkipInBetween() {
Review Comment:
The only difference of this test from `testTwoLevelDecorrelate` is you
removed `d1.deptno = e1.deptno` condition, and I'm not sure what is the meaning
for `SkipInBetween` in the test name, and the reason why we need this test, can
you explain it a bit?
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -866,6 +866,12 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ // Only consider the correlated variables which originated from this
sub-query level.
+ if (!filter.getVariablesSet().isEmpty()) {
+ // Only retain when filter has non-empty correlated variables set to
avoid removing
+ // all the correlated variables from parent scope.
Review Comment:
This comment explains why the `if (...)` exists, how about moving it above
the `if (...)`.
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -3573,4 +3573,48 @@ or 20 in (
!ok
+# Test case for CALCITE-5683 which throws an exception during the
de-correlation phase
+SELECT d1.dname, d1.deptno +
+ ( SELECT max(e1.empno) FROM emp e1
Review Comment:
Usually we start a new line for `FROM` clause.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -6823,6 +6823,50 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule
rule) {
.checkUnchanged();
}
+ /** Test case for CALCITE-5683 for two level nested decorrelate with
standard program
+ * failing during the decorrelation phase. */
+ @Test void testTwoLevelDecorrelate() {
+ final String sql = "SELECT d1.name, d1.deptno + "
+ + " ( SELECT e1.empno "
+ + " FROM emp e1 "
+ + " WHERE d1.deptno = e1.deptno and "
+ + " e1.sal = (SELECT max(sal) "
+ + " FROM emp e2 "
+ + " WHERE e1.sal = e2.sal and"
+ + " e1.deptno = e2.deptno and"
+ + " d1.deptno < e2.deptno))"
+ + " FROM dept d1";
+
+ sql(sql)
+ .withExpand(false)
+ .withLateDecorrelate(true)
+ .withSubQueryRules()
+ .withTrim(true)
+ .withRule()
Review Comment:
`withSubQueryRules` will be overridden by `withRule`, so the test actually
has no rules (the content in the *.xml file also shows that the SubQuery is
actually not removed). Can you change this to something like
`withSubQueryRules().withLateDecorrelate().withTrim().check()`.
NIT: `withExpand(false)` is not needed now since it's false by default now.
--
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]
