You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by am...@apache.org on 2017/03/20 15:56:57 UTC
[4/7] drill git commit: DRILL-5330: NPE in
FunctionImplementationRegistry
DRILL-5330: NPE in FunctionImplementationRegistry
Fixes:
* DRILL-5330: NPE in
FunctionImplementationRegistry.functionReplacement()
* DRILL-5331:
NPE in FunctionImplementationRegistry.findDrillFunction() if dynamic
UDFs disabled
When running in a unit test, the dynamic UDF (DUDF) mechanism is not
available. When running in production, the DUDF mechanism is available,
but may be disabled.
One confusing aspect of this code is that the function registry
is given the option manager, but the option manager is not yet valid
(not yet initialized) in the function registry constructor. So, we
cannot access the option manager in the function registry constructor.
In any event, the existing system options cannot be used to disable DUDF
support. For obscure reasons, DUDF support is always enabled, even when
disabled by the user.
Instead, for DRILL-5331, we added a config option to "really" disable DUDFS.
The property is set only for tests, disables DUDF support.
Note that, in the future, this option could be generalized to
"off, read-only, on" to capture the full set of DUDF modes.
But, for now, just turning this off is sufficient.
For DRILL-5330, we use an existing option validator rather than
accessing the raw option directly.
Also includes a bit of code cleanup in the class in question.
The result is that the code now works when used in a sub-operator unit
test.
close apache/drill#777
Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/2f13c08f
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/2f13c08f
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/2f13c08f
Branch: refs/heads/master
Commit: 2f13c08f35152639e4619d2898b2ca8fe7115259
Parents: 90bc800
Author: Paul Rogers <pr...@maprtech.com>
Authored: Fri Mar 10 11:55:13 2017 -0800
Committer: Aman Sinha <as...@maprtech.com>
Committed: Mon Mar 20 08:09:57 2017 -0700
----------------------------------------------------------------------
.../org/apache/drill/exec/ExecConstants.java | 3 +-
.../coord/store/TransientStoreListener.java | 2 +-
.../expr/fn/FunctionImplementationRegistry.java | 82 +++++++++++++-------
.../src/main/resources/drill-module.conf | 3 +
.../java/org/apache/drill/exec/ExecTest.java | 2 +-
.../exec/physical/impl/TestSimpleFunctions.java | 4 +-
6 files changed, 60 insertions(+), 36 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
index da3a312..91498fc 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
@@ -144,6 +144,7 @@ public interface ExecConstants {
String UDF_DIRECTORY_STAGING = "drill.exec.udf.directory.staging";
String UDF_DIRECTORY_REGISTRY = "drill.exec.udf.directory.registry";
String UDF_DIRECTORY_TMP = "drill.exec.udf.directory.tmp";
+ String UDF_DISABLE_DYNAMIC = "drill.exec.udf.disable_dynamic";
/**
* Local temporary directory is used as base for temporary storage of Dynamic UDF jars.
@@ -264,7 +265,7 @@ public interface ExecConstants {
SLICE_TARGET_DEFAULT);
String CAST_TO_NULLABLE_NUMERIC = "drill.exec.functions.cast_empty_string_to_null";
- OptionValidator CAST_TO_NULLABLE_NUMERIC_OPTION = new BooleanValidator(CAST_TO_NULLABLE_NUMERIC, false);
+ BooleanValidator CAST_TO_NULLABLE_NUMERIC_OPTION = new BooleanValidator(CAST_TO_NULLABLE_NUMERIC, false);
/**
* HashTable runtime settings
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java b/exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
index ca8fa9d..3cd86f9 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
@@ -27,6 +27,6 @@ public interface TransientStoreListener {
*
* @param event event details
*/
- void onChange(TransientStoreEvent event);
+ void onChange(TransientStoreEvent<?> event);
}
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
index 5c7bfb4..c1ba2d8 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
@@ -83,17 +83,30 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
private boolean deleteTmpDir = false;
private File tmpDir;
private List<PluggableFunctionRegistry> pluggableFuncRegistries = Lists.newArrayList();
- private OptionManager optionManager = null;
+ private final OptionManager optionManager;
+ private final boolean useDynamicUdfs;
- @Deprecated @VisibleForTesting
- public FunctionImplementationRegistry(DrillConfig config){
- this(config, ClassPathScanner.fromPrescan(config));
+ @VisibleForTesting
+ public FunctionImplementationRegistry(DrillConfig config) {
+ this(config, ClassPathScanner.fromPrescan(config), null);
}
- public FunctionImplementationRegistry(DrillConfig config, ScanResult classpathScan){
+ public FunctionImplementationRegistry(DrillConfig config, ScanResult classpathScan) {
+ this(config, classpathScan, null);
+ }
+
+ public FunctionImplementationRegistry(DrillConfig config, ScanResult classpathScan, OptionManager optionManager) {
Stopwatch w = Stopwatch.createStarted();
logger.debug("Generating function registry.");
+ this.optionManager = optionManager;
+
+ // Unit tests fail if dynamic UDFs are turned on AND the test happens
+ // to access an undefined function. Since we want a reasonable failure
+ // rather than a crash, we provide a boot-time option, set only by
+ // tests, to disable DUDF lookup.
+
+ useDynamicUdfs = ! config.getBoolean(ExecConstants.UDF_DISABLE_DYNAMIC);
localFunctionRegistry = new LocalFunctionRegistry(classpathScan);
Set<Class<? extends PluggableFunctionRegistry>> registryClasses =
@@ -123,11 +136,6 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
this.localUdfDir = getLocalUdfDir(config);
}
- public FunctionImplementationRegistry(DrillConfig config, ScanResult classpathScan, OptionManager optionManager) {
- this(config, classpathScan);
- this.optionManager = optionManager;
- }
-
/**
* Register functions in given operator table.
* @param operatorTable operator table
@@ -142,7 +150,7 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
}
/**
- * First attempts to finds the Drill function implementation that matches the name, arg types and return type.
+ * First attempts to find the Drill function implementation that matches the name, arg types and return type.
* If exact function implementation was not found,
* syncs local function registry with remote function registry if needed
* and tries to find function implementation one more time
@@ -156,17 +164,25 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
public DrillFuncHolder findDrillFunction(FunctionResolver functionResolver, FunctionCall functionCall) {
AtomicLong version = new AtomicLong();
String newFunctionName = functionReplacement(functionCall);
- List<DrillFuncHolder> functions = localFunctionRegistry.getMethods(newFunctionName, version);
- FunctionResolver exactResolver = FunctionResolverFactory.getExactResolver(functionCall);
- DrillFuncHolder holder = exactResolver.getBestMatch(functions, functionCall);
- if (holder == null) {
+ // Dynamic UDFS: First try with exact match. If not found, we may need to
+ // update the registry, so sync with remote.
+
+ if (useDynamicUdfs) {
+ List<DrillFuncHolder> functions = localFunctionRegistry.getMethods(newFunctionName, version);
+ FunctionResolver exactResolver = FunctionResolverFactory.getExactResolver(functionCall);
+ DrillFuncHolder holder = exactResolver.getBestMatch(functions, functionCall);
+ if (holder != null) {
+ return holder;
+ }
syncWithRemoteRegistry(version.get());
- List<DrillFuncHolder> updatedFunctions = localFunctionRegistry.getMethods(newFunctionName, version);
- holder = functionResolver.getBestMatch(updatedFunctions, functionCall);
}
- return holder;
+ // Whether Dynamic UDFs or not: look in the registry for
+ // an inexact match.
+
+ List<DrillFuncHolder> functions = localFunctionRegistry.getMethods(newFunctionName, version);
+ return functionResolver.getBestMatch(functions, functionCall);
}
/**
@@ -177,16 +193,20 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
*/
private String functionReplacement(FunctionCall functionCall) {
String funcName = functionCall.getName();
- if (functionCall.args.size() > 0) {
- MajorType majorType = functionCall.args.get(0).getMajorType();
- DataMode dataMode = majorType.getMode();
- MinorType minorType = majorType.getMinorType();
- if (optionManager != null
- && optionManager.getOption(ExecConstants.CAST_TO_NULLABLE_NUMERIC).bool_val
- && CastFunctions.isReplacementNeeded(funcName, minorType)) {
- funcName = CastFunctions.getReplacingCastFunction(funcName, dataMode, minorType);
- }
- }
+ if (functionCall.args.size() == 0) {
+ return funcName;
+ }
+ boolean castToNullableNumeric = optionManager != null &&
+ optionManager.getOption(ExecConstants.CAST_TO_NULLABLE_NUMERIC_OPTION);
+ if (! castToNullableNumeric) {
+ return funcName;
+ }
+ MajorType majorType = functionCall.args.get(0).getMajorType();
+ DataMode dataMode = majorType.getMode();
+ MinorType minorType = majorType.getMinorType();
+ if (CastFunctions.isReplacementNeeded(funcName, minorType)) {
+ funcName = CastFunctions.getReplacingCastFunction(funcName, dataMode, minorType);
+ }
return funcName;
}
@@ -200,7 +220,7 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
* @return exactly matching function holder
*/
public DrillFuncHolder findExactMatchingDrillFunction(String name, List<MajorType> argTypes, MajorType returnType) {
- return findExactMatchingDrillFunction(name, argTypes, returnType, true);
+ return findExactMatchingDrillFunction(name, argTypes, returnType, useDynamicUdfs);
}
/**
@@ -315,6 +335,7 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
* @param version remote function registry local function registry was based on
* @return true if remote and local function registries were synchronized after given version
*/
+ @SuppressWarnings("resource")
public boolean syncWithRemoteRegistry(long version) {
if (isRegistrySyncNeeded(remoteFunctionRegistry.getRegistryVersion(), localFunctionRegistry.getVersion())) {
synchronized (this) {
@@ -495,6 +516,7 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
* @return local path to jar that was copied
* @throws IOException in case of problems during jar coping process
*/
+ @SuppressWarnings("resource")
private Path copyJarToLocal(String jarName, RemoteFunctionRegistry remoteFunctionRegistry) throws IOException {
Path registryArea = remoteFunctionRegistry.getRegistryArea();
FileSystem fs = remoteFunctionRegistry.getFs();
@@ -549,7 +571,7 @@ public class FunctionImplementationRegistry implements FunctionLookupContext, Au
private class UnregistrationListener implements TransientStoreListener {
@Override
- public void onChange(TransientStoreEvent event) {
+ public void onChange(TransientStoreEvent<?> event) {
String jarName = (String) event.getValue();
localFunctionRegistry.unregister(jarName);
String localDir = localUdfDir.toUri().getPath();
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/main/resources/drill-module.conf
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/resources/drill-module.conf b/exec/java-exec/src/main/resources/drill-module.conf
index 9c2ba2f..3d66d19 100644
--- a/exec/java-exec/src/main/resources/drill-module.conf
+++ b/exec/java-exec/src/main/resources/drill-module.conf
@@ -245,6 +245,9 @@ drill.exec: {
},
udf: {
retry-attempts: 5,
+ // Disables (parts of) the dynamic UDF functionality.
+ // Primarily for testing.
+ disable_dynamic: false,
directory: {
// Base directory for remote and local udf directories, unique among clusters.
base: ${drill.exec.zk.root}"/udf",
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java
index 4872909..dead858 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java
@@ -52,7 +52,7 @@ public class ExecTest extends DrillTest {
GuavaPatcher.patch();
}
- private static final DrillConfig c = DrillConfig.create();
+ protected static final DrillConfig c = DrillConfig.create();
@After
public void clear(){
http://git-wip-us.apache.org/repos/asf/drill/blob/2f13c08f/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
index ede30e6..6c48651 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
@@ -61,11 +61,10 @@ import com.sun.codemodel.JClassAlreadyExistsException;
import mockit.Injectable;
public class TestSimpleFunctions extends ExecTest {
- //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestSimpleFunctions.class);
- private final DrillConfig c = DrillConfig.create();
@Test
public void testHashFunctionResolution() throws JClassAlreadyExistsException, IOException {
+ @SuppressWarnings("resource")
final FunctionImplementationRegistry registry = new FunctionImplementationRegistry(c);
// test required vs nullable Int input
resolveHash(c,
@@ -133,7 +132,6 @@ public class TestSimpleFunctions extends ExecTest {
FunctionImplementationRegistry registry) throws JClassAlreadyExistsException, IOException {
final List<LogicalExpression> args = new ArrayList<>();
args.add(arg);
- final String[] registeredNames = { "hash" };
FunctionCall call = new FunctionCall(
"hash",
args,