[CALCITE-7442] Getting Wrong index of Correlated variable inside Subquery after FilterJoinRule#4840
[CALCITE-7442] Getting Wrong index of Correlated variable inside Subquery after FilterJoinRule#4840yashlimbad wants to merge 1 commit intoapache:mainfrom
Conversation
53f3132 to
fb20a82
Compare
|
There is an error in the CI that needs to be resolved. |
fb20a82 to
e6ebfcc
Compare
|
Updated the Code. |
|
There was a problem hiding this comment.
Pull request overview
Fixes a decorrelation/planning correctness issue (CALCITE-7442) where a correlated variable’s field index inside a subquery can become incorrect after FilterJoinRule pushes filters.
Changes:
- Add a regression test that exercises
FILTER_INTO_JOIN+FILTER_SUB_QUERY_TO_CORRELATEand then decorrelates, asserting expected plans. - Extend
FilterJoinRuleto propagate correlation-variable sets when creating pushed-down filters (and when keeping an above-join filter). - Enhance
RelOptUtil.classifyFiltersshifting logic so correlated field accesses insideRexSubQuery.relcan be adjusted during filter pushdown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java | Adds a regression test covering correlated variable index behavior through filter pushdown + decorrelation. |
| core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java | Tracks correlation variables when constructing new Filters after pushdown. |
| core/src/main/java/org/apache/calcite/plan/RelOptUtil.java | Extends filter-shifting to also adjust correlated field accesses inside subqueries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java
Outdated
Show resolved
Hide resolved
|
I'm not sure if these comments will be helpful, as I'm not very familiar with this specific area. If someone more knowledgeable doesn't review this PR within the next few days, I'll give it a try myself. |
|
some comments were looking helpful so I updated code now will check by running build if it goes through and push accordingly! 😄 it's fine if someone reviews code in few days, but I feel @julianhyde would be the best to review this because I see only his commit on RelOptUtil which I updated which is called from FilterJoinRule |
2c9af44 to
815c57d
Compare
|
sometimes the tests passes sometimes fails randomly with below error in CI even tho |
815c57d to
ca190f4
Compare
e35441e to
55e4b1d
Compare
|
Hey @xiedeyantu , |
|
Sorry, could I get back to you in a few days? I'm currently on vacation and don't have access to a computer for debugging. |
|
sure, no problem! |
|
@yashlimbad |
|
my bad. updated @caicancai ! |
| * @param adjustments the amount to adjust each field by | ||
| * @param offset the amount to shift field accesses by when | ||
| * rewriting correlated subqueries | ||
| * @param correlateVariableChild the child relation providing the |
There was a problem hiding this comment.
The code comment format here is strange.
There was a problem hiding this comment.
the variable name is big here, that's why it's going into it's doc. any suggestions on formatting?
There was a problem hiding this comment.
Indeed, I'll take a look at other Calcite code later to see if there are any good solutions.
There was a problem hiding this comment.
Perhaps changing to a shorter, more concise variable name would solve the problem. 🤔
There was a problem hiding this comment.
got it, will get back on this tomorrow
There was a problem hiding this comment.
using a shorter variable name to make formatting of comments nice is not a good reason.
There was a problem hiding this comment.
Thanks for the reminder, I'll look into whether there's a better way to handle this tomorrow.
| @Override public RexNode visitSubQuery(RexSubQuery subQuery) { | ||
| boolean[] update = {false}; | ||
| List<RexNode> clonedOperands = visitList(subQuery.operands, update); | ||
| if (update[0]) { |
There was a problem hiding this comment.
I suspect there might be an issue with the EXISTS subquery, but I'm not entirely sure. Could you add a similar test?
| subQuery = subQuery.clone(subQuery.getType(), clonedOperands); | ||
| final Set<CorrelationId> variablesSet = RelOptUtil.getVariablesUsed(subQuery.rel); | ||
| if (!variablesSet.isEmpty() && correlateVariableChild != null) { | ||
| CorrelationId id = Iterables.getOnlyElement(variablesSet); |
There was a problem hiding this comment.
Are you assuming there's only one correlation variable?



Jira Link
CALCITE-7442
Changes
Adjust offset of correlated variable inside subquery when pushing filter via FilterJoinRule.java