DRILL-6089: Removed ordering trait from HashJoin in planner and verified the planner does not assume HashJoin preserves ordering.
closes #1117 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/24a7acd4 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/24a7acd4 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/24a7acd4 Branch: refs/heads/master Commit: 24a7acd440d29b7340a378306f339e1307892208 Parents: 20185c9 Author: Timothy Farkas <timothyfar...@apache.org> Authored: Fri Jan 26 13:46:22 2018 -0800 Committer: Vitalii Diravka <vitalii.dira...@gmail.com> Committed: Fri Feb 16 20:32:54 2018 +0000 ---------------------------------------------------------------------- .../exec/planner/physical/JoinPruleBase.java | 10 +++--- .../drill/exec/planner/physical/PrelUtil.java | 15 ++++++++ .../java/org/apache/drill/PlanTestBase.java | 37 +++++++++++++++----- .../impl/join/TestHashJoinAdvanced.java | 16 +++++++-- 4 files changed, 60 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/24a7acd4/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/JoinPruleBase.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/JoinPruleBase.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/JoinPruleBase.java index 80e8dda..8b0d69a 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/JoinPruleBase.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/JoinPruleBase.java @@ -116,7 +116,6 @@ public abstract class JoinPruleBase extends Prule { } } - // Create join plan with both left and right children hash distributed. If the physical join type // is MergeJoin, a collation must be provided for both left and right child and the plan will contain // sort converter if necessary to provide the collation. @@ -126,8 +125,6 @@ public abstract class JoinPruleBase extends Prule { RelCollation collationLeft, RelCollation collationRight, DrillDistributionTrait hashLeftPartition, DrillDistributionTrait hashRightPartition) throws InvalidRelException { - //DrillDistributionTrait hashLeftPartition = new DrillDistributionTrait(DrillDistributionTrait.DistributionType.HASH_DISTRIBUTED, ImmutableList.copyOf(getDistributionField(join.getLeftKeys()))); - //DrillDistributionTrait hashRightPartition = new DrillDistributionTrait(DrillDistributionTrait.DistributionType.HASH_DISTRIBUTED, ImmutableList.copyOf(getDistributionField(join.getRightKeys()))); RelTraitSet traitsLeft = null; RelTraitSet traitsRight = null; @@ -146,7 +143,8 @@ public abstract class JoinPruleBase extends Prule { DrillJoinRelBase newJoin = null; if (physicalJoinType == PhysicalJoinType.HASH_JOIN) { - newJoin = new HashJoinPrel(join.getCluster(), traitsLeft, + final RelTraitSet traitSet = PrelUtil.removeCollation(traitsLeft, call); + newJoin = new HashJoinPrel(join.getCluster(), traitSet, convertedLeft, convertedRight, join.getCondition(), join.getJoinType()); @@ -236,7 +234,8 @@ public abstract class JoinPruleBase extends Prule { call.transformTo(new MergeJoinPrel(join.getCluster(), convertedLeft.getTraitSet(), convertedLeft, convertedRight, joinCondition, join.getJoinType())); } else if (physicalJoinType == PhysicalJoinType.HASH_JOIN) { - call.transformTo(new HashJoinPrel(join.getCluster(), convertedLeft.getTraitSet(), convertedLeft, + final RelTraitSet traitSet = PrelUtil.removeCollation(convertedLeft.getTraitSet(), call); + call.transformTo(new HashJoinPrel(join.getCluster(), traitSet, convertedLeft, convertedRight, joinCondition, join.getJoinType())); } else if (physicalJoinType == PhysicalJoinType.NESTEDLOOP_JOIN) { call.transformTo(new NestedLoopJoinPrel(join.getCluster(), convertedLeft.getTraitSet(), convertedLeft, @@ -245,5 +244,4 @@ public abstract class JoinPruleBase extends Prule { } } - } http://git-wip-us.apache.org/repos/asf/drill/blob/24a7acd4/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java index 67b3066..9c0ee40 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java @@ -25,6 +25,7 @@ import java.util.Set; import org.apache.calcite.plan.RelOptCluster; import org.apache.calcite.plan.RelOptPlanner; import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelTrait; import org.apache.calcite.plan.RelTraitSet; import org.apache.calcite.rel.RelCollation; import org.apache.calcite.rel.RelFieldCollation; @@ -327,6 +328,20 @@ public class PrelUtil { } } + // DRILL-6089 make sure no collations are added to HashJoin + public static RelTraitSet removeCollation(RelTraitSet traitSet, RelOptRuleCall call) + { + RelTraitSet newTraitSet = call.getPlanner().emptyTraitSet(); + + for (RelTrait trait: traitSet) { + if (!trait.getTraitDef().getTraitClass().equals(RelCollation.class)) { + newTraitSet = newTraitSet.plus(trait); + } + } + + return newTraitSet; + } + public static class InputRefRemap { private int oldIndex; private int newIndex; http://git-wip-us.apache.org/repos/asf/drill/blob/24a7acd4/exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java b/exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java index 22b734b..47a8623 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java +++ b/exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java @@ -80,28 +80,47 @@ public class PlanTestBase extends BaseTestQuery { * planning process throws an exception */ public static void testPlanMatchingPatterns(String query, String[] expectedPatterns, String[] excludedPatterns) - throws Exception { + throws Exception { + testPlanMatchingPatterns(query, stringsToPatterns(expectedPatterns), stringsToPatterns(excludedPatterns)); + } + + public static void testPlanMatchingPatterns(String query, Pattern[] expectedPatterns, Pattern[] excludedPatterns) + throws Exception { final String plan = getPlanInString("EXPLAIN PLAN for " + QueryTestUtil.normalizeQuery(query), OPTIQ_FORMAT); // Check and make sure all expected patterns are in the plan if (expectedPatterns != null) { - for (final String s : expectedPatterns) { - final Pattern p = Pattern.compile(s); - final Matcher m = p.matcher(plan); - assertTrue(EXPECTED_NOT_FOUND + s +"\n" + plan, m.find()); + for (final Pattern expectedPattern: expectedPatterns) { + final Matcher m = expectedPattern.matcher(plan); + assertTrue(EXPECTED_NOT_FOUND + expectedPattern.pattern() +"\n" + plan, m.find()); } } // Check and make sure all excluded patterns are not in the plan if (excludedPatterns != null) { - for (final String s : excludedPatterns) { - final Pattern p = Pattern.compile(s); - final Matcher m = p.matcher(plan); - assertFalse(UNEXPECTED_FOUND + s +"\n" + plan, m.find()); + for (final Pattern excludedPattern: excludedPatterns) { + final Matcher m = excludedPattern.matcher(plan); + assertFalse(UNEXPECTED_FOUND + excludedPattern.pattern() +"\n" + plan, m.find()); } } } + private static Pattern[] stringsToPatterns(String[] strings) + { + if (strings == null) { + return null; + } + + final Pattern[] patterns = new Pattern[strings.length]; + + for (int index = 0; index < strings.length; index++) { + final String string = strings[index]; + patterns[index] = Pattern.compile(string); + } + + return patterns; + } + /** * Runs an explain plan including attributes query and check for expected regex patterns * (in optiq text format), also ensure excluded patterns are not found. Either list can http://git-wip-us.apache.org/repos/asf/drill/blob/24a7acd4/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java index b9b97c1..d4a7814 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java @@ -18,7 +18,6 @@ package org.apache.drill.exec.physical.impl.join; - import org.apache.drill.categories.OperatorTest; import org.apache.drill.categories.UnlikelyTest; import org.apache.drill.exec.ExecConstants; @@ -31,8 +30,7 @@ import java.io.BufferedWriter; import java.io.File; import java.io.FileWriter; import java.nio.file.Paths; -import java.util.concurrent.ExecutorService; - +import java.util.regex.Pattern; @Category(OperatorTest.class) public class TestHashJoinAdvanced extends JoinTestBase { @@ -41,6 +39,8 @@ public class TestHashJoinAdvanced extends JoinTestBase { @BeforeClass public static void disableMergeJoin() throws Exception { dirTestWatcher.copyResourceToRoot(Paths.get("join", "empty_part")); + dirTestWatcher.copyFileToRoot(Paths.get("sample-data", "region.parquet")); + dirTestWatcher.copyFileToRoot(Paths.get("sample-data", "nation.parquet")); test(DISABLE_MJ); } @@ -197,4 +197,14 @@ public class TestHashJoinAdvanced extends JoinTestBase { BaseTestQuery.resetSessionOption(ExecConstants.SLICE_TARGET); } } + + @Test // DRILL-6089 + public void testJoinOrdering() throws Exception { + final String query = "select * from dfs.`sample-data/nation.parquet` nation left outer join " + + "(select * from dfs.`sample-data/region.parquet`) " + + "as region on region.r_regionkey = nation.n_nationkey order by nation.n_name desc"; + + final Pattern sortHashJoinPattern = Pattern.compile(".*Sort.*HashJoin", Pattern.DOTALL); + testPlanMatchingPatterns(query, new Pattern[]{sortHashJoinPattern}, null); + } }
