You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "rubenada (via GitHub)" <gi...@apache.org> on 2023/06/21 09:03:58 UTC
[GitHub] [calcite] rubenada opened a new pull request, #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
rubenada opened a new pull request, #3277:
URL: https://github.com/apache/calcite/pull/3277
See details in CALCITE-5789
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3277:
URL: https://github.com/apache/calcite/pull/3277#discussion_r1242366055
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -865,6 +866,9 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (!filterVariablesSet.isEmpty()) {
+ variablesSet.retainAll(filterVariablesSet);
Review Comment:
I agree and would give a big thanks to you for doing 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3277:
URL: https://github.com/apache/calcite/pull/3277#issuecomment-1602282308
@libenchao sorry to disturb, you have recently reviewed other PRs concerning `SubQueryRemoveRule`, do you mind taking a look at this one?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a diff in pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3277:
URL: https://github.com/apache/calcite/pull/3277#discussion_r1242360002
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -865,6 +866,9 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (!filterVariablesSet.isEmpty()) {
+ variablesSet.retainAll(filterVariablesSet);
Review Comment:
Thanks @libenchao .
I just saw on [PR#3193](https://github.com/apache/calcite/pull/3193) that @HanumathRao is off traveling these days, so I propose to do the other way around: I integrated in the current PR the changes from PR#3193 (including your review from [here](https://github.com/libenchao/calcite/commits/5683-nested-correlated-sub-query-throws-2)), tagging @HanumathRao as co-author. We can merge all changes for both tickets ([CALCITE-5789](https://issues.apache.org/jira/browse/CALCITE-5789) & [CALCITE-5683](https://issues.apache.org/jira/browse/CALCITE-5683)) under the current PR, we close PR#3193 (superseded by this one), and I'll reopen [CALCITE-5716](https://issues.apache.org/jira/browse/CALCITE-5716) which will remain a pending issue.
If you agree, I'll squash commits and proceed to merge
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3277:
URL: https://github.com/apache/calcite/pull/3277#discussion_r1242064409
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -865,6 +866,9 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (!filterVariablesSet.isEmpty()) {
+ variablesSet.retainAll(filterVariablesSet);
Review Comment:
Agreed, thanks for the detailed explanation, we're on the same page.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3277:
URL: https://github.com/apache/calcite/pull/3277#issuecomment-1607771822
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3277)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list)
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a diff in pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3277:
URL: https://github.com/apache/calcite/pull/3277#discussion_r1241254889
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -865,6 +866,9 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (!filterVariablesSet.isEmpty()) {
+ variablesSet.retainAll(filterVariablesSet);
Review Comment:
@libenchao I have the impression that `if (!filterVariablesSet.isEmpty()) { variablesSet.retainAll(filterVariablesSet); }` it's better than the current code, since fixes some issues (like the current one), but the proper fix would be just `variablesSet.retainAll(filterVariablesSet);` (without the `if` check).
When applying the fix without the `if` I got one regression in `misc.iq`, but this is something that already appeared in https://github.com/apache/calcite/pull/3193 and that PR contains the fix for it (see [this comment](https://github.com/apache/calcite/pull/3193#discussion_r1202473255))
I also found one new test case that is currently broken, and will only be fixed if we apply the patch without the ìf`:
```
select deptno from dept d1 where exists (
select 1 from dept d2 where d2.deptno = d1.deptno and exists (
select 1 from dept d3 where d3.dname = d1.dname));
```
It is a bit "forced", because the query can be easily re-written to remove the inner-most subquery (which does not require to be 2-level nested); but it is a valid (although rare) scenario, which should ideally work (but since it is a bit far-fectched and it has a straightforward workaround by rewriting the query, I didn't put too much attention on it).
The thing is, and it seems you and I both reached the same conclusion (as you explain [here](https://github.com/apache/calcite/pull/3193#discussion_r1204993079)), if we apply the patch without the `if`, I fear we can lead to some issues on downstream projects, because the logic to initialize the variableSet of Filter is a bit fragile, and downstream projects using RelBuilder API can create a (wrong, incomplete) Filter that has a condition with RexSubQuery with a correlation variable, without setting this variable in the Filter's variableSet. Thus, when the `filterVariablesSet` is not empty, we know that is was correctly initialized, so we can exploit that info; but when it is empty, we can never know if it is correct (i.e. it is empty because it is supposed to be empty), or it is incorrect (it is supposed to be **not** empty, but the Filter was "wrongly" created without a variableSet). So, at that point, if we exploit that info (e.g. by applying the fix without the `if`), things c
an't stop working in the second case.
That's why I thought it was safer to include the "partial fix" (retain only if `filterVariablesSet` is not empty), even though it will not fix certain cases (like the SQL query that I mentioned earlier, or the "last test" that you mention on your branch, which actually comes from a different ticket [CALCITE-5716](https://issues.apache.org/jira/browse/CALCITE-5716) ).
I would propose (given than 1.35 RC phase is approaching) a conservative approach:
- I can close this PR & ticket, because it is superseded by https://github.com/apache/calcite/pull/3193 (we could perhaps add my new test in `sub-query.iq` from this PR into that PR, just for completeness).
- To avoid regressions on downstream projects, https://github.com/apache/calcite/pull/3193 would apply the fix with the `if`; that should fix [CALCITE-5683](https://issues.apache.org/jira/browse/CALCITE-5683) (and my ticket [CALCITE-5789](https://issues.apache.org/jira/browse/CALCITE-5789)); but it shall not contain the "last test in sub-query.iq` (which would be broken because AFAIK it requires the fix without the `if`).
- In any case, that test actually belongs to a different (related) issue: [CALCITE-5716](https://issues.apache.org/jira/browse/CALCITE-5716). That ticket was closed, but IMO it should be re-open; and we would need to re-evaluate whether or not fix it (and the risks it may provoke) by removing the `if` from the patch (in a future version).
wdyt? (sorry for the long message)
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3277:
URL: https://github.com/apache/calcite/pull/3277#discussion_r1240592799
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -865,6 +866,9 @@ private static void matchFilter(SubQueryRemoveRule rule,
LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(c), e);
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (!filterVariablesSet.isEmpty()) {
+ variablesSet.retainAll(filterVariablesSet);
Review Comment:
The change looks good to me, and this was also introduced in https://github.com/apache/calcite/pull/3193, (it seems that the author reverts this change in the latest version).
But I found after this change, there is one test that produces incorrect result, see my latest comment(https://github.com/apache/calcite/pull/3193#issuecomment-1596028987). I'm not sure whether it's a regression, if not, I think we can safely merge this one.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3277:
URL: https://github.com/apache/calcite/pull/3277#issuecomment-1600497197
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3277)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list)
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3277:
URL: https://github.com/apache/calcite/pull/3277#issuecomment-1607731671
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3277)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3277&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3277&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3277&metric=new_duplicated_lines_density&view=list)
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada merged pull request #3277: [CALCITE-5789] Query with two nested subqueries where the inner-most references the outer-most table returns wrong result
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada merged PR #3277:
URL: https://github.com/apache/calcite/pull/3277
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org