-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-23799 : Calcite. Hash join. #11770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ExecutionTest.java # modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/JoinCommutePlannerTest.java
This reverts commit 3337e98.
...ain/java/org/apache/ignite/internal/processors/query/calcite/rule/HashJoinConverterRule.java
Show resolved
Hide resolved
|
|
||
| /** Tests 'Select e.id, e.name, d.name as dep_name from DEP d <JOIN TYPE> join EMP e on e.DEPNO = d.DEPNO'. */ | ||
| @Test | ||
| public void testHashJoin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExecutionTest already contains a lot of unsorted tests, maybe it's time to move join tests to another class. But we also have MergeJoinExecutionTest/NestedLoopJoinExecutionTest they look almost identical and looks like they cover more cases than testHashJoin test. Maybe it's better to extract common part and add new HashJoinExecutionTest class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right. But this might be other tests refactoring ticket.
...internal/processors/query/calcite/integration/CalciteBasicSecondaryIndexIntegrationTest.java
Outdated
Show resolved
Hide resolved
...st/java/org/apache/ignite/internal/processors/query/calcite/planner/HashJoinPlannerTest.java
Outdated
Show resolved
Hide resolved
...st/java/org/apache/ignite/internal/processors/query/calcite/planner/HashJoinPlannerTest.java
Outdated
Show resolved
Hide resolved
| private static List<List<Object>> testJoinIsAppliedParameters() { | ||
| return F.asList( | ||
| F.asList("select t1.c1 from t1 %s join t1 t2 using(c1)", true), | ||
| F.asList("select t1.c1 from t1 %s join t1 t2 on t1.c1 = t2.c1", true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add two columns test.
Also, in cases like t1.c1 = t2.c2 and t1.id = 1 second condition can be pushed down to table scan (at least for some types of join), perhaps these cases should be tested too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two-column tests below. Yes, some scans are pushed down and join uses only one equi join pair.
...a/org/apache/ignite/internal/processors/query/calcite/planner/JoinColocationPlannerTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java
Outdated
Show resolved
Hide resolved
# Conflicts: # modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ExecutionTest.java # modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/JoinCommutePlannerTest.java
# Conflicts: # modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java
# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/RuntimeHashIndex.java # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/NestedLoopJoinNode.java # modules/calcite/src/test/java/org/apache/ignite/testsuites/ExecutionTestSuite.java
# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/MergeJoinConverterRule.java
...te/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteHashJoin.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/processors/query/calcite/exec/MappingRowHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/processors/query/calcite/exec/MappingRowHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/processors/query/calcite/exec/MappingRowHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/HashJoinNode.java
Outdated
Show resolved
Hide resolved
| RowT right = rightIt.next(); | ||
|
|
||
| if (nonEqCond != null && !nonEqCond.test(left, right)) | ||
| continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can block the thread for a long time here, not allowing other queries to proceed. We should leave after some count of rows and schedule the next task (the same for other join types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But may block thread similary in many places. We could take known https://issues.apache.org/jira/browse/IGNITE-23960 later and adjust it. WDYT?
| return false; | ||
|
|
||
| // IS NOT DISTINCT is currently not supported simultaneously with equi conditions. | ||
| if (joinInfo.hasMatchingNulls() && joinInfo.matchingNullsCnt() != joinInfo.pairs().size()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't understand how it worked for example for JoinIntegrationTest#testIsNotDistinctWithEquiConditionFrom (and other) it's two key pairs in the test (IS NOT DISTINCT FROM and EQUALS) and other join converter rules are disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't work. There were no these tests yet.
No description provided.