From f43de99bcb3c7098e0865735f2398f49f1ef9d86 Mon Sep 17 00:00:00 2001 From: Arina Ielchiieva Date: Tue, 2 Feb 2016 18:48:29 +0200 Subject: [PATCH] DRILL-3944: Drill MAXDIR Unknown variable or type FILE_SEPARATOR --- .../codegen/templates/DirectoryExplorers.java | 3 +- .../parser/UnsupportedOperatorsVisitor.java | 73 ++++++++++++++- .../planner/TestDirectoryExplorerUDFs.java | 89 +++++++++++++++---- 3 files changed, 147 insertions(+), 18 deletions(-) diff --git a/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java b/exec/java-exec/src/main/codegen/templates/DirectoryExplorers.java index c97e34b0690..655ff811ede 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 @@ */ 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 void eval() { 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); 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 17bc3393ee9..e90aa3b5684 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.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 disabledType = Lists.newArrayList(); private static List disabledOperators = Lists.newArrayList(); + private static List 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 SqlNode visit(SqlCall sqlCall) { } } - // 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 SqlNode visit(SqlCall sqlCall) { "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 SqlNode visit(SqlCall sqlCall) { "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 @@ private void checkGrouping(SqlSelect sqlSelect) { } } + 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. * @@ -402,6 +444,35 @@ public boolean test(SqlNode sqlNode) { } }; + /** + * 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 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. 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 516f9758dbd..b830f488e4e 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 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 ConstantFoldingTestConfig(String funcName, String expectedFolderName) { } } - @Test - public void testConstExprFolding_maxDir0() throws Exception { - - new TestConstantFolding.SmallFileCreator(folder).createFiles(1, 1000); - String path = folder.getRoot().toPath().toString(); + private static List 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 void testConstExprFolding_maxDir0() throws Exception { // 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 tests = ImmutableList.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.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 allFiles = ImmutableList.builder() .add("smallfile") @@ -104,4 +122,45 @@ public void testConstExprFolding_maxDir0() throws Exception { .go(); } + @Test + public void testIncorrectFunctionPlacement() throws Exception { + + Map configMap = ImmutableMap.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 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;"); + } + } }