IGNITE-24196 Sql. Merge projection into join node #5398
Conversation
...l-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java
Outdated
Show resolved
Hide resolved
...l-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java
Show resolved
Hide resolved
- replaced `joinNeedsProjection` method with enumSet - make `joinWithProjectionOnTopReturnsValidaResults` test sensible to changes in `createJoinProjection`
| @ParameterizedTest | ||
| @MethodSource("joinTypesWithRulesToDisable") | ||
| @SuppressWarnings("ConcatenationWithEmptyString") | ||
| void joinWithProjectionOnTopReturnsValidaResults(JoinType joinType, List<String> rulesToDisable) { |
There was a problem hiding this comment.
| void joinWithProjectionOnTopReturnsValidaResults(JoinType joinType, List<String> rulesToDisable) { | |
| void joinWithProjectionOnTopReturnsValidResults(JoinType joinType, List<String> rulesToDisable) { |
| checker.check(); | ||
| } | ||
|
|
||
| private static Stream<Arguments> joinTypesWithRulesToDisable() { |
There was a problem hiding this comment.
I suppose this is second reincarnation of ItAggregatesTest#makePermutations - if so, can we merge them ?
There was a problem hiding this comment.
we won't get rid of joinTypesWithRulesToDisable because it creates permutation of join algorithms with combinations of rules to disable while ItAggregatesTest#makePermutations only creates combination of rules to disable. We can replace three-line stream
List<String> rulesToDisable = joinConverterRules.stream()
.filter(ruleName -> !joinAlgo.equals(ruleName))
.collect(Collectors.toList())
with a single method call, but the amount of changes required to refactor ItAggregatesTest#makePermutations as library function simply makes the effort doesn't worth it.
If you insists, I would propose to do it as follow up ticket though
There was a problem hiding this comment.
No, i`m not insist, plz proceed
...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItJoinOrderTest.java
Show resolved
Hide resolved
- typo
# Conflicts: # modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/ExecutionTest.java
https://issues.apache.org/jira/browse/IGNITE-24196
This patch introduces new
SqlJoinProjectionexpression to use instead ofRowFactory#concatin join nodes. Besides, in reorganizes schema initialization inItJoinOrderTestto take an advantage of DDL batching.Results of SqlJoinBenchmark on my laptop as follow:
Also I run query 64 from TPC-DS with sf=1 to make sure there is no degradation:
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes