aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorVolodymyr Vysotskyi <vvovyk@gmail.com>2019-03-10 13:57:03 +0200
committerSorabh Hamirwasia <sorabh@apache.org>2019-03-14 15:36:11 -0700
commit5aa38a51d90998234b4ca46434ce225df72addc5 (patch)
tree46c8777ef8cd139b7f285b8df7cbed7d4cb4cb85
parente7f0bbf1f45b0d39d3796be81207aecd716d6aae (diff)
downloaddrill-5aa38a51d90998234b4ca46434ce225df72addc5.tar.gz
DRILL-2326: Fix scalar replacement for the case when static method which does not return values is called
- Fix check for return function value to handle the case when created object is returned without assigning it to the local variable closes #1687
-rw-r--r--contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java35
-rw-r--r--exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java24
-rw-r--r--exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java7
-rw-r--r--exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java130
4 files changed, 75 insertions, 121 deletions
diff --git a/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java b/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java
index 43c3e3f9a..86d11f24a 100644
--- a/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java
+++ b/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java
@@ -18,6 +18,7 @@
package org.apache.drill.exec.fn.hive;
import java.time.LocalDateTime;
+import java.util.Arrays;
import java.util.List;
import org.apache.commons.lang3.tuple.Pair;
@@ -25,10 +26,10 @@ import org.apache.drill.categories.HiveStorageTest;
import org.apache.drill.categories.SlowTest;
import org.apache.drill.common.expression.SchemaPath;
import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.compile.ClassCompilerSelector;
import org.apache.drill.exec.compile.ClassTransformer;
import org.apache.drill.exec.hive.HiveTestBase;
-import org.apache.drill.exec.server.options.OptionValue;
-import org.apache.drill.test.QueryTestUtil;
import org.apache.drill.test.TestBuilder;
import org.junit.Test;
import org.junit.experimental.categories.Category;
@@ -114,33 +115,31 @@ public class TestInbuiltHiveUDFs extends HiveTestBase {
.go();
}
- @Test //DRILL-4868
+ @Test //DRILL-4868 & DRILL-2326
public void testEmbeddedHiveFunctionCall() throws Exception {
- // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ClassTransformer.ScalarReplacementOption.TRY);
+ String query =
+ "SELECT convert_from(unhex(key2), 'INT_BE') as intkey \n" +
+ "FROM cp.`functions/conv/conv.json`";
+
+ List<String> compilers = Arrays.asList(ClassCompilerSelector.CompilerPolicy.JANINO.name(),
+ ClassCompilerSelector.CompilerPolicy.JDK.name());
try {
- final String[] queries = {
- "SELECT convert_from(unhex(key2), 'INT_BE') as intkey \n" +
- "FROM cp.`functions/conv/conv.json`",
- };
+ setSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION, ClassTransformer.ScalarReplacementOption.ON.name());
+ for (String compilerName : compilers) {
+ setSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION, compilerName);
- for (String query: queries) {
testBuilder()
.sqlQuery(query)
- .ordered()
+ .unOrdered()
.baselineColumns("intkey")
- .baselineValues(1244739896)
- .baselineValues(new Object[] { null })
- .baselineValues(1313814865)
- .baselineValues(1852782897)
+ .baselineValuesForSingleColumn(1244739896, null, 1313814865, 1852782897)
.build()
.run();
}
-
} finally {
- // restore the system option
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
+ resetSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION);
+ resetSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION);
}
}
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
index 71c222b61..291cf6baa 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
@@ -434,23 +434,25 @@ public class InstructionModifier extends MethodVisitor {
*
* Does the function being called return a holder?
*/
- final ReplacingBasicValue functionReturn = getFunctionReturn();
- if (functionReturn != null) {
- /*
- * The return of this method is an actual instance of the object we're escaping.
- * Update so that it gets mapped correctly.
- */
- super.visitMethodInsn(opcode, owner, name, desc, itf);
- functionReturn.markFunctionReturn();
- return;
+ if (Type.getReturnType(desc) != Type.VOID_TYPE) {
+ ReplacingBasicValue functionReturn = getFunctionReturn();
+ if (functionReturn != null) {
+ /*
+ * The return of this method is an actual instance of the object we're escaping.
+ * Update so that it gets mapped correctly.
+ */
+ super.visitMethodInsn(opcode, owner, name, desc, itf);
+ functionReturn.markFunctionReturn();
+ return;
+ }
}
/*
* Holders can't be passed as arguments to methods, because their contents aren't
* maintained; we use the locals instead. Therefore, complain if any arguments are holders.
*/
- for(int argDepth = argCount - 1; argDepth >= 0; --argDepth) {
- final ReplacingBasicValue argValue = peekFromTop(argDepth);
+ for (int argDepth = argCount - 1; argDepth >= 0; --argDepth) {
+ ReplacingBasicValue argValue = peekFromTop(argDepth);
if (argValue != null) {
throw new IllegalStateException(
String.format("Holder types are not allowed to be passed between methods. " +
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
index 13063c8a5..a34854dbc 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
@@ -83,6 +83,13 @@ public class ReplacingInterpreter extends BasicInterpreter {
}
@Override
+ public void returnOperation(AbstractInsnNode insn, BasicValue value, BasicValue expected) {
+ if (value instanceof ReplacingBasicValue) {
+ ((ReplacingBasicValue) value).markFunctionReturn();
+ }
+ }
+
+ @Override
public BasicValue unaryOperation(final AbstractInsnNode insn, final BasicValue value)
throws AnalyzerException {
/*
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java
index d2e0020c3..1bc5f550a 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java
@@ -22,7 +22,6 @@ import static org.apache.drill.test.TestBuilder.mapOf;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.io.BufferedWriter;
@@ -31,26 +30,23 @@ import java.io.FileWriter;
import java.time.LocalDate;
import java.time.LocalTime;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.List;
import org.apache.drill.categories.UnlikelyTest;
import org.apache.drill.exec.ExecConstants;
-import org.apache.drill.exec.compile.ClassTransformer.ScalarReplacementOption;
+import org.apache.drill.exec.compile.ClassCompilerSelector;
+import org.apache.drill.exec.compile.ClassTransformer;
import org.apache.drill.exec.compile.CodeCompiler;
import org.apache.drill.exec.expr.fn.impl.DateUtility;
import org.apache.drill.exec.proto.UserBitShared.QueryType;
import org.apache.drill.exec.record.RecordBatchLoader;
-import org.apache.drill.exec.rpc.RpcException;
import org.apache.drill.exec.rpc.user.QueryDataBatch;
-import org.apache.drill.exec.server.options.OptionValue;
import org.apache.drill.exec.util.ByteBufUtil.HadoopWritables;
-import org.apache.drill.exec.util.VectorUtil;
import org.apache.drill.exec.vector.ValueVector;
import org.apache.drill.exec.vector.VarCharVector;
import org.apache.drill.test.BaseTestQuery;
-import org.apache.drill.test.QueryTestUtil;
import org.junit.BeforeClass;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.experimental.categories.Category;
@@ -73,10 +69,10 @@ public class TestConvertFunctions extends BaseTestQuery {
private static LocalTime time = LocalTime.parse("01:23:45.678", DateUtility.getTimeFormatter());
private static LocalDate date = LocalDate.parse("1980-01-01", DateUtility.getDateTimeFormatter());
- String textFileContent;
+ private String textFileContent;
@BeforeClass
- public static void setup( ) {
+ public static void setup() {
// Tests here rely on the byte-code merge approach to code
// generation and will fail if using plain-old Java.
// Actually, some queries succeed with plain-old Java that
@@ -94,31 +90,26 @@ public class TestConvertFunctions extends BaseTestQuery {
@Test // DRILL-3854
public void testConvertFromConvertToInt() throws Exception {
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.OFF);
+ String newTblName = "testConvertFromConvertToInt_tbl";
try {
- final String newTblName = "testConvertFromConvertToInt_tbl";
+ setSessionOption(ExecConstants.SLICE_TARGET, 1);
- test("alter session set `planner.slice_target` = 1");
test("CREATE TABLE dfs.%s as \n" +
"SELECT convert_to(r_regionkey, 'INT') as ct \n" +
"FROM cp.`tpch/region.parquet`", newTblName);
+
testBuilder()
.sqlQuery("SELECT convert_from(ct, 'INT') as cf \n" +
"FROM dfs.%s \n" +
"ORDER BY ct", newTblName)
- .ordered()
+ .unOrdered()
.baselineColumns("cf")
- .baselineValues(0)
- .baselineValues(1)
- .baselineValues(2)
- .baselineValues(3)
- .baselineValues(4)
+ .baselineValuesForSingleColumn(0, 1, 2, 3, 4)
.build()
.run();
} finally {
- // restore the system option
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
- test("alter session set `planner.slice_target` = " + ExecConstants.SLICE_TARGET_DEFAULT);
+ resetSessionOption(ExecConstants.SLICE_TARGET);
+ test("drop table if exists dfs.%s", newTblName);
}
}
@@ -536,68 +527,25 @@ public class TestConvertFunctions extends BaseTestQuery {
verifyPhysicalPlan("convert_to('apache_drill', 'UTF8')", new byte[] {'a', 'p', 'a', 'c', 'h', 'e', '_', 'd', 'r', 'i', 'l', 'l'});
}
- @Ignore // TODO(DRILL-2326) remove this when we get rid of the scalar replacement option test cases below
- @Test
+ @Test // DRILL-2326
public void testBigIntVarCharReturnTripConvertLogical() throws Exception {
- final String logicalPlan = Resources.toString(
+ String logicalPlan = Resources.toString(
Resources.getResource(CONVERSION_TEST_LOGICAL_PLAN), Charsets.UTF_8);
- final List<QueryDataBatch> results = testLogicalWithResults(logicalPlan);
- int count = 0;
- final RecordBatchLoader loader = new RecordBatchLoader(getAllocator());
- for (QueryDataBatch result : results) {
- count += result.getHeader().getRowCount();
- loader.load(result.getHeader().getDef(), result.getData());
- if (loader.getRecordCount() > 0) {
- VectorUtil.logVectorAccessibleContent(loader);
- }
- loader.clear();
- result.release();
- }
- assertTrue(count == 10);
- }
+ List<String> compilers = Arrays.asList(ClassCompilerSelector.CompilerPolicy.JANINO.name(),
+ ClassCompilerSelector.CompilerPolicy.JDK.name());
- @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
- public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceTRY() throws Exception {
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.TRY);
- try {
- // this should work fine
- testBigIntVarCharReturnTripConvertLogical();
- } finally {
- // restore the system option
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
- }
- }
-
- @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
- @Ignore // Because this test sometimes fails, sometimes succeeds
- public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceON() throws Exception {
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.ON);
- boolean caughtException = false;
try {
- // this used to fail (with a JUnit assertion) until we fix the SR bug
- // Something in DRILL-5116 seemed to fix this problem, so the test now
- // succeeds - sometimes.
- testBigIntVarCharReturnTripConvertLogical();
- } catch(RpcException e) {
- caughtException = true;
- } finally {
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
- }
-
- // Yes: sometimes this works, sometimes it does not...
- assertTrue(!caughtException || caughtException);
- }
+ setSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION, ClassTransformer.ScalarReplacementOption.ON.name());
+ for (String compilerName : compilers) {
+ setSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION, compilerName);
- @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
- public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceOFF() throws Exception {
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.OFF);
- try {
- // this should work fine
- testBigIntVarCharReturnTripConvertLogical();
+ int count = testRunAndPrint(QueryType.LOGICAL, logicalPlan);
+ assertEquals(10, count);
+ }
} finally {
- // restore the system option
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
+ resetSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION);
+ resetSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION);
}
}
@@ -642,33 +590,31 @@ public class TestConvertFunctions extends BaseTestQuery {
buffer.release();
}
- @Test // DRILL-4862
+ @Test // DRILL-4862 & DRILL-2326
public void testBinaryString() throws Exception {
- // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
- final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.TRY);
+ String query =
+ "SELECT convert_from(binary_string(key), 'INT_BE') as intkey \n" +
+ "FROM cp.`functions/conv/conv.json`";
+
+ List<String> compilers = Arrays.asList(ClassCompilerSelector.CompilerPolicy.JANINO.name(),
+ ClassCompilerSelector.CompilerPolicy.JDK.name());
try {
- final String[] queries = {
- "SELECT convert_from(binary_string(key), 'INT_BE') as intkey \n" +
- "FROM cp.`functions/conv/conv.json`"
- };
+ setSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION, ClassTransformer.ScalarReplacementOption.ON.name());
+ for (String compilerName : compilers) {
+ setSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION, compilerName);
- for (String query: queries) {
testBuilder()
.sqlQuery(query)
- .ordered()
+ .unOrdered()
.baselineColumns("intkey")
- .baselineValues(1244739896)
- .baselineValues(new Object[] { null })
- .baselineValues(1313814865)
- .baselineValues(1852782897)
+ .baselineValuesForSingleColumn(1244739896, null, 1313814865, 1852782897)
.build()
.run();
}
-
} finally {
- // restore the system option
- QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
+ resetSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION);
+ resetSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION);
}
}