Repository: drill Updated Branches: refs/heads/master 3e55fe1db -> b9960f89e
DRILL-3944: Drill MAXDIR Unknown variable or type FILE_SEPARATOR. This closes #391 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/cca17cb8 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/cca17cb8 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/cca17cb8 Branch: refs/heads/master Commit: cca17cb8f9891bb0b49560acad7f129a6cb7edc0 Parents: 3e55fe1 Author: Arina Ielchiieva <[email protected]> Authored: Tue Feb 2 18:48:29 2016 +0200 Committer: Parth Chandra <[email protected]> Committed: Fri Feb 26 09:52:38 2016 -0800 ---------------------------------------------------------------------- .../codegen/templates/DirectoryExplorers.java | 3 +- .../sql/parser/UnsupportedOperatorsVisitor.java | 73 +++++++++++++++- .../exec/planner/TestDirectoryExplorerUDFs.java | 89 ++++++++++++++++---- 3 files changed, 147 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/cca17cb8/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java b/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java index c97e34b..655ff81 100644 --- a/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java +++ b/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java @@ -38,7 +38,6 @@ import javax.inject.Inject; */ public class DirectoryExplorers { static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DirectoryExplorers.class); - private static final String FILE_SEPARATOR = "/"; <#list [ { "name" : "\"maxdir\"", "functionClassName" : "MaxDir", "comparison" : "compareTo(curr) < 0", "goal" : "maximum", "comparisonType" : "case-sensitive"}, { "name" : "\"imaxdir\"", "functionClassName" : "IMaxDir", "comparison" : "compareToIgnoreCase(curr) < 0", "goal" : "maximum", "comparisonType" : "case-insensitive"}, @@ -94,7 +93,7 @@ public class DirectoryExplorers { subPartitionStr = curr; } } - String[] subPartitionParts = subPartitionStr.split(FILE_SEPARATOR); + String[] subPartitionParts = subPartitionStr.split("/"); subPartitionStr = subPartitionParts[subPartitionParts.length - 1]; byte[] result = subPartitionStr.getBytes(); out.buffer = buffer = buffer.reallocIfNeeded(result.length); http://git-wip-us.apache.org/repos/asf/drill/blob/cca17cb8/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java ---------------------------------------------------------------------- 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 17bc339..e90aa3b 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 @@ -24,6 +24,7 @@ import org.apache.calcite.sql.util.SqlBasicVisitor; import org.apache.drill.exec.ExecConstants; import org.apache.drill.exec.exception.UnsupportedOperatorCollector; import org.apache.drill.exec.ops.QueryContext; +import org.apache.drill.exec.planner.physical.PlannerSettings; import org.apache.drill.exec.work.foreman.SqlUnsupportedException; import org.apache.calcite.sql.SqlSelectKeyword; @@ -48,12 +49,17 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { private QueryContext context; private static List<String> disabledType = Lists.newArrayList(); private static List<String> disabledOperators = Lists.newArrayList(); + private static List<String> dirExplorers = Lists.newArrayList(); static { disabledType.add(SqlTypeName.TINYINT.name()); disabledType.add(SqlTypeName.SMALLINT.name()); disabledType.add(SqlTypeName.REAL.name()); disabledOperators.add("CARDINALITY"); + dirExplorers.add("MAXDIR"); + dirExplorers.add("IMAXDIR"); + dirExplorers.add("MINDIR"); + dirExplorers.add("IMINDIR"); } private UnsupportedOperatorCollector unsupportedOperatorCollector; @@ -269,9 +275,29 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { } } - // Disable complex functions being present in any place other than Select-Clause + // Disable complex functions incorrect placement if(sqlCall instanceof SqlSelect) { SqlSelect sqlSelect = (SqlSelect) sqlCall; + + for (SqlNode nodeInSelectList : sqlSelect.getSelectList()) { + if (checkDirExplorers(nodeInSelectList)) { + unsupportedOperatorCollector.setException(SqlUnsupportedException.ExceptionType.FUNCTION, + "Directory explorers " + dirExplorers + " functions are not supported in Select List\n" + + "See Apache Drill JIRA: DRILL-3944"); + throw new UnsupportedOperationException(); + } + } + + if (sqlSelect.hasWhere()) { + if (checkDirExplorers(sqlSelect.getWhere()) && !context.getPlannerSettings().isConstantFoldingEnabled()) { + unsupportedOperatorCollector.setException(SqlUnsupportedException.ExceptionType.FUNCTION, + "Directory explorers " + dirExplorers + " functions can not be used " + + "when " + PlannerSettings.CONSTANT_FOLDING.getOptionName() + " option is set to false\n" + + "See Apache Drill JIRA: DRILL-3944"); + throw new UnsupportedOperationException(); + } + } + if(sqlSelect.hasOrderBy()) { for (SqlNode sqlNode : sqlSelect.getOrderList()) { if(containsFlatten(sqlNode)) { @@ -279,6 +305,11 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { "Flatten function is not supported in Order By\n" + "See Apache Drill JIRA: DRILL-2181"); throw new UnsupportedOperationException(); + } else if (checkDirExplorers(sqlNode)) { + unsupportedOperatorCollector.setException(SqlUnsupportedException.ExceptionType.FUNCTION, + "Directory explorers " + dirExplorers + " functions are not supported in Order By\n" + + "See Apache Drill JIRA: DRILL-3944"); + throw new UnsupportedOperationException(); } } } @@ -290,6 +321,11 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { "Flatten function is not supported in Group By\n" + "See Apache Drill JIRA: DRILL-2181"); throw new UnsupportedOperationException(); + } else if (checkDirExplorers(sqlNode)) { + unsupportedOperatorCollector.setException(SqlUnsupportedException.ExceptionType.FUNCTION, + "Directory explorers " + dirExplorers + " functions are not supported in Group By\n" + + "See Apache Drill JIRA: DRILL-3944"); + throw new UnsupportedOperationException(); } } } @@ -351,6 +387,12 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { } } + private boolean checkDirExplorers(SqlNode sqlNode) { + final ExprFinder dirExplorersFinder = new ExprFinder(DirExplorersCondition); + sqlNode.accept(dirExplorersFinder); + return dirExplorersFinder.find(); + } + /** * A function that replies true or false for a given expression. * @@ -403,6 +445,35 @@ public class UnsupportedOperatorsVisitor extends SqlShuttle { }; /** + * A condition that returns true if SqlNode has Directory Explorers. + */ + private final SqlNodeCondition DirExplorersCondition = new SqlNodeCondition() { + @Override + public boolean test(SqlNode sqlNode) { + return sqlNode instanceof SqlCall && checkOperator((SqlCall) sqlNode, dirExplorers, true); + } + + /** + * Checks recursively if operator and its operands are present in provided list of operators + */ + private boolean checkOperator(SqlCall sqlCall, List<String> operators, boolean checkOperator) { + if (checkOperator) { + return operators.contains(sqlCall.getOperator().getName().toUpperCase()) || checkOperator(sqlCall, operators, false); + } + for (SqlNode sqlNode : sqlCall.getOperandList()) { + if (!(sqlNode instanceof SqlCall)) { + continue; + } + if (checkOperator((SqlCall) sqlNode, operators, true)) { + return true; + } + } + return false; + } + + }; + + /** * A visitor to check if the given SqlNodeCondition is tested as true or not. * If the condition is true, mark flag 'find' as true. */ http://git-wip-us.apache.org/repos/asf/drill/blob/cca17cb8/exec/java-exec/src/test/java/org/apache/drill/exec/planner/TestDirectoryExplorerUDFs.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/planner/TestDirectoryExplorerUDFs.java b/exec/java-exec/src/test/java/org/apache/drill/exec/planner/TestDirectoryExplorerUDFs.java index 516f975..b830f48 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/planner/TestDirectoryExplorerUDFs.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/planner/TestDirectoryExplorerUDFs.java @@ -18,11 +18,16 @@ package org.apache.drill.exec.planner; import java.util.List; +import java.util.Map; +import com.google.common.collect.ImmutableMap; import org.apache.drill.PlanTestBase; +import org.apache.drill.common.exceptions.UserRemoteException; import org.apache.drill.exec.fn.interp.TestConstantFolding; import org.apache.drill.exec.util.JsonStringArrayList; import org.apache.drill.exec.util.Text; +import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -30,12 +35,12 @@ import org.junit.rules.TemporaryFolder; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; -public class TestDirectoryExplorerUDFs extends PlanTestBase { +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertThat; - @Rule - public TemporaryFolder folder = new TemporaryFolder(); +public class TestDirectoryExplorerUDFs extends PlanTestBase { - private class ConstantFoldingTestConfig { + private static class ConstantFoldingTestConfig { String funcName; String expectedFolderName; public ConstantFoldingTestConfig(String funcName, String expectedFolderName) { @@ -44,14 +49,14 @@ public class TestDirectoryExplorerUDFs extends PlanTestBase { } } - @Test - public void testConstExprFolding_maxDir0() throws Exception { - - new TestConstantFolding.SmallFileCreator(folder).createFiles(1, 1000); - String path = folder.getRoot().toPath().toString(); + private static List<ConstantFoldingTestConfig> tests; + private String path; - test("use dfs.root"); + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + @BeforeClass + public static void init() { // Need the suffixes to make the names unique in the directory. // The capitalized name is on the opposite function (imaxdir and mindir) // because they are looking on opposite ends of the list. @@ -60,12 +65,25 @@ public class TestDirectoryExplorerUDFs extends PlanTestBase { // first in the case-sensitive ordering. // SMALLFILE_2 comes last in a case-insensitive ordering because it has // a suffix not found on smallfile. - List<ConstantFoldingTestConfig> tests = ImmutableList.<ConstantFoldingTestConfig>builder() - .add(new ConstantFoldingTestConfig("maxdir", "smallfile")) - .add(new ConstantFoldingTestConfig("imaxdir", "SMALLFILE_2")) - .add(new ConstantFoldingTestConfig("mindir", "BIGFILE_2")) - .add(new ConstantFoldingTestConfig("imindir", "bigfile")) + tests = ImmutableList.<ConstantFoldingTestConfig>builder() + .add(new ConstantFoldingTestConfig("MAXDIR", "smallfile")) + .add(new ConstantFoldingTestConfig("IMAXDIR", "SMALLFILE_2")) + .add(new ConstantFoldingTestConfig("MINDIR", "BIGFILE_2")) + .add(new ConstantFoldingTestConfig("IMINDIR", "bigfile")) .build(); + } + + @Before + public void setup() throws Exception { + new TestConstantFolding.SmallFileCreator(folder).createFiles(1, 1000); + path = folder.getRoot().toPath().toString(); + } + + + @Test + public void testConstExprFolding_maxDir0() throws Exception { + + test("use dfs.root"); List<String> allFiles = ImmutableList.<String>builder() .add("smallfile") @@ -104,4 +122,45 @@ public class TestDirectoryExplorerUDFs extends PlanTestBase { .go(); } + @Test + public void testIncorrectFunctionPlacement() throws Exception { + + Map<String, String> configMap = ImmutableMap.<String, String>builder() + .put("select %s('dfs.root','" + path + "') from dfs.`" + path + "/*/*.csv`", "Select List") + .put("select dir0 from dfs.`" + path + "/*/*.csv` order by %s('dfs.root','" + path + "')", "Order By") + .put("select max(dir0) from dfs.`" + path + "/*/*.csv` group by %s('dfs.root','" + path + "')", "Group By") + .put("select concat(concat(%s('dfs.root','" + path + "'),'someName'),'someName') from dfs.`" + path + "/*/*.csv`", "Select List") + .put("select dir0 from dfs.`" + path + "/*/*.csv` order by concat(%s('dfs.root','" + path + "'),'someName')", "Order By") + .put("select max(dir0) from dfs.`" + path + "/*/*.csv` group by concat(%s('dfs.root','" + path + "'),'someName')", "Group By") + .build(); + + for (Map.Entry<String, String> configEntry : configMap.entrySet()) { + for (ConstantFoldingTestConfig functionConfig : tests) { + try { + test(String.format(configEntry.getKey(), functionConfig.funcName)); + } catch (UserRemoteException e) { + assertThat(e.getMessage(), containsString( + String.format("Directory explorers [MAXDIR, IMAXDIR, MINDIR, IMINDIR] functions are not supported in %s", configEntry.getValue()))); + } + } + } + } + + @Test + public void testConstantFoldingOff() throws Exception { + try { + test("set `planner.enable_constant_folding` = false;"); + String query = "select * from dfs.`" + path + "/*/*.csv` where dir0 = %s('dfs.root','" + path + "')"; + for (ConstantFoldingTestConfig config : tests) { + try { + test(String.format(query, config.funcName)); + } catch (UserRemoteException e) { + assertThat(e.getMessage(), containsString("Directory explorers [MAXDIR, IMAXDIR, MINDIR, IMINDIR] functions can not be used " + + "when planner.enable_constant_folding option is set to false")); + } + } + } finally { + test("set `planner.enable_constant_folding` = true;"); + } + } }
