This is an automated email from the ASF dual-hosted git repository. vitalii pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/drill.git
commit cab059abc0c0f6044e9aa15a1747e8542ac5ad46 Author: Igor Guzenko <[email protected]> AuthorDate: Wed Oct 3 14:55:31 2018 +0300 DRILL-786: Allow CROSS JOIN syntax 1. Removed throw statement in UnsupportedOperatorsVisitor 2. Extended UnsupportedRelOperatorException's message closes #1488 --- .../drill/exec/physical/impl/join/JoinUtils.java | 30 +++ .../planner/sql/handlers/DefaultSqlHandler.java | 14 +- .../sql/parser/UnsupportedOperatorsVisitor.java | 9 - .../apache/drill/TestDisabledFunctionality.java | 10 +- .../drill/exec/planner/sql/CrossJoinTest.java | 201 +++++++++++++++++++++ 5 files changed, 238 insertions(+), 26 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/JoinUtils.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/JoinUtils.java index 52871e2..90e8558 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/JoinUtils.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/JoinUtils.java @@ -51,11 +51,14 @@ import org.apache.drill.exec.ops.FragmentContext; import org.apache.drill.exec.planner.logical.DrillLimitRel; import org.apache.drill.exec.record.VectorAccessible; import org.apache.drill.exec.resolver.TypeCastRules; +import org.apache.drill.exec.work.foreman.UnsupportedRelOperatorException; import java.util.ArrayList; import java.util.LinkedList; import java.util.List; +import static org.apache.drill.exec.planner.physical.PlannerSettings.NLJOIN_FOR_SCALAR; + public class JoinUtils { public enum JoinCategory { @@ -65,6 +68,11 @@ public class JoinUtils { } private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(JoinUtils.class); + public static final String FAILED_TO_PLAN_CARTESIAN_JOIN = String.format( + "This query cannot be planned possibly due to either a cartesian join or an inequality join. %n" + + "If a cartesian or inequality join is used intentionally, set the option '%s' to false and try again.", + NLJOIN_FOR_SCALAR.getOptionName()); + // Check the comparator is supported in join condition. Note that a similar check is also // done in JoinPrel; however we have to repeat it here because a physical plan // may be submitted directly to Drill. @@ -128,6 +136,18 @@ public class JoinUtils { } /** + * Check if the given RelNode contains any Cartesian join. + * Return true if find one. Otherwise, return false. + * + * @param relNode {@link RelNode} instance to be inspected + * @return Return true if the given relNode contains Cartesian join. + * Otherwise, return false + */ + public static boolean checkCartesianJoin(RelNode relNode) { + return checkCartesianJoin(relNode, new LinkedList<>(), new LinkedList<>(), new LinkedList<>()); + } + + /** * Checks if implicit cast is allowed between the two input types of the join condition. Currently we allow * implicit casts in join condition only between numeric types and varchar/varbinary types. * @param input1 @@ -300,6 +320,16 @@ public class JoinUtils { } /** + * Creates new exception for queries that cannot be planned due + * to presence of cartesian or inequality join. + * + * @return new {@link UnsupportedRelOperatorException} instance + */ + public static UnsupportedRelOperatorException cartesianJoinPlanningException() { + return new UnsupportedRelOperatorException(FAILED_TO_PLAN_CARTESIAN_JOIN); + } + + /** * Collects expressions list from the input project. * For the case when input rel node has single input, its input is taken. */ diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java index c75311f..f7d11f8 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java @@ -18,7 +18,6 @@ package org.apache.drill.exec.planner.sql.handlers; import java.io.IOException; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.concurrent.TimeUnit; @@ -106,7 +105,6 @@ import org.apache.drill.exec.server.options.OptionManager; import org.apache.drill.exec.util.Pointer; import org.apache.drill.exec.work.foreman.ForemanSetupException; import org.apache.drill.exec.work.foreman.SqlUnsupportedException; -import org.apache.drill.exec.work.foreman.UnsupportedRelOperatorException; import org.slf4j.Logger; import org.apache.drill.shaded.guava.com.google.common.base.Preconditions; @@ -294,8 +292,8 @@ public class DefaultSqlHandler extends AbstractSqlHandler { } catch (RelOptPlanner.CannotPlanException ex) { logger.error(ex.getMessage()); - if (JoinUtils.checkCartesianJoin(relNode, new ArrayList<>(), new ArrayList<>(), new ArrayList<>())) { - throw new UnsupportedRelOperatorException("This query cannot be planned possibly due to either a cartesian join or an inequality join"); + if (JoinUtils.checkCartesianJoin(relNode)) { + throw JoinUtils.cartesianJoinPlanningException(); } else { throw ex; } @@ -459,8 +457,8 @@ public class DefaultSqlHandler extends AbstractSqlHandler { } catch (RelOptPlanner.CannotPlanException ex) { logger.error(ex.getMessage()); - if (JoinUtils.checkCartesianJoin(drel, new ArrayList<>(), new ArrayList<>(), new ArrayList<>())) { - throw new UnsupportedRelOperatorException("This query cannot be planned possibly due to either a cartesian join or an inequality join"); + if (JoinUtils.checkCartesianJoin(drel)) { + throw JoinUtils.cartesianJoinPlanningException(); } else { throw ex; } @@ -482,8 +480,8 @@ public class DefaultSqlHandler extends AbstractSqlHandler { } catch (RelOptPlanner.CannotPlanException ex) { logger.error(ex.getMessage()); - if (JoinUtils.checkCartesianJoin(drel, new ArrayList<>(), new ArrayList<>(), new ArrayList<>())) { - throw new UnsupportedRelOperatorException("This query cannot be planned possibly due to either a cartesian join or an inequality join"); + if (JoinUtils.checkCartesianJoin(drel)) { + throw JoinUtils.cartesianJoinPlanningException(); } else { throw ex; } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java index 8505a68..b972841 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java @@ -36,7 +36,6 @@ import org.apache.calcite.sql.fun.SqlCountAggFunction; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlJoin; -import org.apache.calcite.sql.JoinType; import org.apache.calcite.sql.SqlNode; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql.util.SqlShuttle; @@ -256,14 +255,6 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { "See Apache Drill JIRA: DRILL-1986"); throw new UnsupportedOperationException(); } - - // Block Cross Join - if(join.getJoinType() == JoinType.CROSS) { - unsupportedOperatorCollector.setException(SqlUnsupportedException.ExceptionType.RELATIONAL, - "CROSS JOIN is not supported\n" + - "See Apache Drill JIRA: DRILL-1921"); - throw new UnsupportedOperationException(); - } } //Disable UNNEST if the configuration disable it diff --git a/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java b/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java index 679d01e..1dcd691 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java +++ b/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java @@ -16,6 +16,7 @@ * limitations under the License. */ package org.apache.drill; + import org.apache.drill.categories.UnlikelyTest; import org.apache.drill.common.exceptions.UserException; import org.apache.drill.exec.planner.physical.PlannerSettings; @@ -92,15 +93,6 @@ public class TestDisabledFunctionality extends BaseTestQuery { } } - @Test(expected = UnsupportedRelOperatorException.class) // see DRILL-1921 - public void testDisabledCrossJoin() throws Exception { - try { - test("select * from cp.`tpch/nation.parquet` CROSS JOIN cp.`tpch/region.parquet`"); - } catch(UserException ex) { - throwAsUnsupportedException(ex); - } - } - @Test(expected = UnsupportedDataTypeException.class) // see DRILL-1959 public void testDisabledCastTINYINT() throws Exception { try { diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/planner/sql/CrossJoinTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/planner/sql/CrossJoinTest.java new file mode 100644 index 0000000..348df32 --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/planner/sql/CrossJoinTest.java @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.planner.sql; + +import org.apache.drill.categories.SqlTest; +import org.apache.drill.common.exceptions.UserRemoteException; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.apache.drill.test.ClusterFixture; +import org.apache.drill.test.ClusterTest; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import static org.apache.drill.exec.physical.impl.join.JoinUtils.FAILED_TO_PLAN_CARTESIAN_JOIN; + +@Category(SqlTest.class) +public class CrossJoinTest extends ClusterTest { + + private static int NATION_TABLE_RECORDS_COUNT = 25; + + private static int EXPECTED_COUNT = NATION_TABLE_RECORDS_COUNT * NATION_TABLE_RECORDS_COUNT; + + @BeforeClass + public static void setUp() throws Exception { + startCluster(ClusterFixture.builder(dirTestWatcher)); + } + + @After + public void tearDown() { + client.resetSession(PlannerSettings.NLJOIN_FOR_SCALAR.getOptionName()); + } + + @Test + public void testCrossJoinFailsForEnabledOption() throws Exception { + enableNlJoinForScalarOnly(); + + thrownException.expect(UserRemoteException.class); + thrownException.expectMessage(FAILED_TO_PLAN_CARTESIAN_JOIN); + + queryBuilder().sql( + "SELECT l.n_name, r.n_name " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS JOIN cp.`tpch/nation.parquet` r") + .run(); + } + + @Test + public void testCrossJoinSucceedsForDisabledOption() throws Exception { + disableNlJoinForScalarOnly(); + client.testBuilder().sqlQuery( + "SELECT l.n_name,r.n_name " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS JOIN cp.`tpch/nation.parquet` r") + .expectsNumRecords(EXPECTED_COUNT) + .go(); + } + + @Test + public void testCommaJoinFailsForEnabledOption() throws Exception { + enableNlJoinForScalarOnly(); + + thrownException.expect(UserRemoteException.class); + thrownException.expectMessage(FAILED_TO_PLAN_CARTESIAN_JOIN); + + queryBuilder().sql( + "SELECT l.n_name,r.n_name " + + "FROM cp.`tpch/nation.parquet` l, cp.`tpch/nation.parquet` r") + .run(); + } + + @Test + public void testCommaJoinSucceedsForDisabledOption() throws Exception { + disableNlJoinForScalarOnly(); + client.testBuilder().sqlQuery( + "SELECT l.n_name,r.n_name " + + "FROM cp.`tpch/nation.parquet` l, cp.`tpch/nation.parquet` r") + .expectsNumRecords(EXPECTED_COUNT) + .go(); + } + + @Test + public void testSubSelectCrossJoinFailsForEnabledOption() throws Exception { + enableNlJoinForScalarOnly(); + + thrownException.expect(UserRemoteException.class); + thrownException.expectMessage(FAILED_TO_PLAN_CARTESIAN_JOIN); + + queryBuilder().sql( + "SELECT COUNT(*) c " + + "FROM (" + + "SELECT l.n_name,r.n_name " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS JOIN cp.`tpch/nation.parquet` r" + + ")") + .run(); + } + + @Test + public void testSubSelectCrossJoinSucceedsForDisabledOption() throws Exception { + disableNlJoinForScalarOnly(); + + client.testBuilder() + .sqlQuery( + "SELECT COUNT(*) c " + + "FROM (SELECT l.n_name,r.n_name " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS JOIN cp.`tpch/nation.parquet` r)") + .unOrdered() + .baselineColumns("c") + .baselineValues((long) EXPECTED_COUNT) + .go(); + } + + @Test + public void textCrossAndCommaJoinFailsForEnabledOption() throws Exception { + enableNlJoinForScalarOnly(); + + thrownException.expect(UserRemoteException.class); + thrownException.expectMessage(FAILED_TO_PLAN_CARTESIAN_JOIN); + + queryBuilder().sql( + "SELECT * " + + "FROM cp.`tpch/nation.parquet` a, cp.`tpch/nation.parquet` b " + + "CROSS JOIN cp.`tpch/nation.parquet` c") + .run(); + } + + @Test + public void textCrossAndCommaJoinSucceedsForDisabledOption() throws Exception { + disableNlJoinForScalarOnly(); + + client.testBuilder().sqlQuery( + "SELECT * " + + "FROM cp.`tpch/nation.parquet` a, cp.`tpch/nation.parquet` b " + + "CROSS JOIN cp.`tpch/nation.parquet` c") + .expectsNumRecords(NATION_TABLE_RECORDS_COUNT * EXPECTED_COUNT) + .go(); + } + + @Test + public void testCrossApplyFailsForEnabledOption() throws Exception { + enableNlJoinForScalarOnly(); + + thrownException.expect(UserRemoteException.class); + thrownException.expectMessage(FAILED_TO_PLAN_CARTESIAN_JOIN); + + queryBuilder().sql( + "SELECT * " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS APPLY cp.`tpch/nation.parquet` r") + .run(); + } + + @Test + public void testCrossApplySucceedsForDisabledOption() throws Exception { + disableNlJoinForScalarOnly(); + + client.testBuilder().sqlQuery( + "SELECT * " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS APPLY cp.`tpch/nation.parquet` r") + .expectsNumRecords(EXPECTED_COUNT) + .go(); + } + + @Test + public void testCrossJoinSucceedsForEnabledOptionAndScalarInput() throws Exception { + enableNlJoinForScalarOnly(); + + client.testBuilder().sqlQuery( + "SELECT * " + + "FROM cp.`tpch/nation.parquet` l " + + "CROSS JOIN (SELECT * FROM cp.`tpch/nation.parquet` r LIMIT 1)") + .expectsNumRecords(NATION_TABLE_RECORDS_COUNT) + .go(); + } + + private static void disableNlJoinForScalarOnly() { + client.alterSession(PlannerSettings.NLJOIN_FOR_SCALAR.getOptionName(), false); + } + + private static void enableNlJoinForScalarOnly() { + client.alterSession(PlannerSettings.NLJOIN_FOR_SCALAR.getOptionName(), true); + } +}
