[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1227350034
##
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:
By SkipInBetween, I actually mean't that the correlation variable is not
used at the first level and directly used at the deeper level (here second
level depth). I wanted to make sure that even if there is no correlation used
in the intermediate levels, the solution works.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1209078369
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -3549,7 +3549,17 @@ protected final void createAggImpl(
// implement HAVING (we have already checked that it is non-trivial)
relBuilder.push(bb.root());
if (havingExpr != null) {
- relBuilder.filter(havingExpr);
+ /* Set the correlation variables used in this sub-query to the filter
node,
+ * same logic is being used for the filter generated in where clause.
+ */
+ Set variableSet = new HashSet<>();
+ if (havingExpr instanceof RexSubQuery) {
Review Comment:
Added the code changes to find the SubQuery and also a test case to exercise
this code path. thanks
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193: URL: https://github.com/apache/calcite/pull/3193#discussion_r1209077887 ## core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java: ## @@ -866,6 +866,8 @@ private static void matchFilter(SubQueryRemoveRule rule, LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e); final Set variablesSet = RelOptUtil.getVariablesUsed(e.rel); + /* Only consider the correlated variables which originated from this sub-query level*/ + variablesSet.retainAll(filter.getVariablesSet()); Review Comment: Yeah, it makes sense to add that changes to be on the safe side. I have changed the code. thanks -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1209076981
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6823,48 @@ 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)
+.withTrim(true)
+.withRule()
+.checkUnchanged();
Review Comment:
OK, thank you for working on it.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1205057374
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6823,48 @@ 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)
+.withTrim(true)
+.withRule()
+.checkUnchanged();
Review Comment:
Yeah, please let me know if it makes sense to remove these tests (as they
actually are redundant) as sub-query.iq is having these test cases and are
reporting an error without the fix.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193: URL: https://github.com/apache/calcite/pull/3193#discussion_r1205054838 ## core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java: ## @@ -866,6 +866,8 @@ private static void matchFilter(SubQueryRemoveRule rule, LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e); final Set variablesSet = RelOptUtil.getVariablesUsed(e.rel); + /* Only consider the correlated variables which originated from this sub-query level*/ + variablesSet.retainAll(filter.getVariablesSet()); Review Comment: Agree that this fix is using the existing mechanism to set the variable in Filter operator, I did have questions like what happens when there other operator nodes etc. I want to handle the specific case for this PR(as you mentioned going in the right direction) and handle as we have more use cases (please let me know if it makes sense). Agree with you that this code might result in a throwing an exception in cases where previous code was generating a plan. However, I encountered a scenario while debugging the JIRA([CALCITE-5716](https://issues.apache.org/jira/browse/CALCITE-5716)) wherein it was not reporting an error but generating a wrong plan. With the fix provided in this PR it is generating a right result (when running a generated plan). Updated the JIRA with the analysis -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1205043890
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -3549,7 +3549,17 @@ protected final void createAggImpl(
// implement HAVING (we have already checked that it is non-trivial)
relBuilder.push(bb.root());
if (havingExpr != null) {
- relBuilder.filter(havingExpr);
+ /* Set the correlation variables used in this sub-query to the filter
node,
+ * same logic is being used for the filter generated in where clause.
+ */
+ Set variableSet = new HashSet<>();
+ if (havingExpr instanceof RexSubQuery) {
Review Comment:
Agree that this might be taking care of limited cases, I didn't want to
increase the scope of this PR. Please let me know if you foresee any use cases
that is missing then I can take care of it in this PR (otherwise I think it
might be better to handle it in another PR).
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1202516946
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -3549,7 +3549,17 @@ protected final void createAggImpl(
// implement HAVING (we have already checked that it is non-trivial)
relBuilder.push(bb.root());
if (havingExpr != null) {
- relBuilder.filter(havingExpr);
+ /* Set the correlation variables used in this sub-query to the filter
node,
+ * same logic is being used for the filter generated in where clause.
+ */
+ Set variableSet = new HashSet<>();
Review Comment:
Without this fix there is an existing test case failure (I think it is in
CoreQuidemTest).
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193: URL: https://github.com/apache/calcite/pull/3193#discussion_r1202515386 ## core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java: ## @@ -866,6 +866,8 @@ private static void matchFilter(SubQueryRemoveRule rule, LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e); final Set variablesSet = RelOptUtil.getVariablesUsed(e.rel); + /* Only consider the correlated variables which originated from this sub-query level*/ + variablesSet.retainAll(filter.getVariablesSet()); Review Comment: I want to make sure if you have applied the fix in both the files SubQueryRemoveRule and SqlToRelConverter.java. I remember that the SqlToRelConverter changes were needed to fix a similar regression not sure if this is same query. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1202371411
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6824,65 @@ 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() {
Review Comment:
@libenchao and @rubenada thanks for reviewing the changes. I have
addressed the review comments, please take a look at it and let me know. thanks.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1192598969
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6824,65 @@ 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() {
Review Comment:
@libenchao Just want to circle back on the review comment.Please let me know
if you have any suggestions. Thanks
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] HanumathRao commented on a diff in pull request #3193: [CALCITE-5683] Two level nested correlated subquery throws an excepti…
HanumathRao commented on code in PR #3193:
URL: https://github.com/apache/calcite/pull/3193#discussion_r1190174369
##
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##
@@ -6823,6 +6824,65 @@ 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() {
Review Comment:
@libenchao Thanks for reviewing the PR. Yeah this test case is different in
style because the issue is showing up only with standard program. Let me know
if there is a better place(file) to add these test cases.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
