You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by arina-ielchiieva <gi...@git.apache.org> on 2017/05/23 13:52:23 UTC
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/843
DRILL-5533: Fix flag assignment in FunctionInitializer.checkInit() method
Changes:
1. Fixed DCL in FunctionInitializer.checkInit() method (update flag parameter when function body is loaded).
2. Fixed ImportGrabber.getImports() method to return the list with imports.
3. Added unit tests for FunctionInitializer.
4, Minor refactoring (renamed methods, added javadoc).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill DRILL-5533
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/843.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #843
----
commit 69e911a37115a4b6d98db88f33557ca5cf2b4e85
Author: Arina Ielchiieva <ar...@gmail.com>
Date: 2017-05-22T14:49:31Z
DRILL-5533: Fix flag assignment in FunctionInitializer.checkInit() method
Changes:
1. Fixed DCL in FunctionInitializer.checkInit() method (update flag parameter when function body is loaded).
2. Fixed ImportGrabber.getImports() method to return the list with imports.
3. Added unit tests for FunctionInitializer.
4, Minor refactoring (renamed methods, added javadoc).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118186352
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java ---
@@ -63,9 +62,21 @@ public void traverseStaticImportOnDemandDeclaration(StaticImportOnDemandDeclarat
}
- public static List<String> getMethods(Java.CompilationUnit cu){
- ImportGrabber visitor = new ImportGrabber();
- cu.getPackageMemberTypeDeclarations()[0].accept(visitor.finder.comprehensiveVisitor());
+ /**
+ * Creates list of imports that are present in compilation unit.
+ * For example:
+ * [import io.netty.buffer.DrillBuf;, import org.apache.drill.exec.expr.DrillSimpleFunc;]
+ *
+ * @param compilationUnit compilation unit
+ * @return list of imports
+ */
+ public static List<String> getImports(Java.CompilationUnit compilationUnit){
--- End diff --
When writing code for UDFs, we recommend users to use the fully qualified class names instead of imports.
What if we enhance our UDFs generation code to use imports from function class? This way, fully qualified class names usage won't be required. Since I thought it could be used for future enhancement, I have left `ImportGrabber`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118184300
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -119,33 +117,35 @@ private void checkInit() {
}
}
- private CompilationUnit get(Class<?> c) throws IOException {
- String path = c.getName();
+ /**
+ * Using class name generates path to class source code (*.java),
+ * reads its content as string and parses it into {@link org.codehaus.janino.Java.CompilationUnit}.
+ *
+ * @param clazz function class
+ * @return compilation unit
+ * @throws IOException if did not find class or could not load it
+ */
+ public CompilationUnit convertToCompilationUnit(Class<?> clazz) throws IOException {
--- End diff --
Sure, did not revert by accident. Thanks for noticing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118186385
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -74,41 +70,43 @@ public String getClassName() {
* @return the imports of this class (for java code gen)
*/
public List<String> getImports() {
- checkInit();
+ loadFunctionBody();
return imports;
}
/**
- * @param methodName
+ * @param methodName method name
* @return the content of the method (for java code gen inlining)
*/
public String getMethod(String methodName) {
- checkInit();
+ loadFunctionBody();
return methods.get(methodName);
}
- private void checkInit() {
- if (ready) {
+ /**
+ * Loads function body: methods (for instance, eval, setup, reset) and imports.
+ * Loading is done once per class instance upon first function invocation.
+ * Double-checked locking is used to avoid concurrency issues
+ * when two threads are trying to load the function body at the same time.
+ */
+ private void loadFunctionBody() {
+ if (isLoaded) {
return;
}
synchronized (this) {
- if (ready) {
+ if (isLoaded) {
return;
}
- // get function body.
-
+ logger.debug("Getting function body for the {}", className);
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118021389
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -119,33 +117,35 @@ private void checkInit() {
}
}
- private CompilationUnit get(Class<?> c) throws IOException {
- String path = c.getName();
+ /**
+ * Using class name generates path to class source code (*.java),
+ * reads its content as string and parses it into {@link org.codehaus.janino.Java.CompilationUnit}.
+ *
+ * @param clazz function class
+ * @return compilation unit
+ * @throws IOException if did not find class or could not load it
+ */
+ public CompilationUnit convertToCompilationUnit(Class<?> clazz) throws IOException {
--- End diff --
private?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118025364
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java ---
@@ -63,9 +62,21 @@ public void traverseStaticImportOnDemandDeclaration(StaticImportOnDemandDeclarat
}
- public static List<String> getMethods(Java.CompilationUnit cu){
- ImportGrabber visitor = new ImportGrabber();
- cu.getPackageMemberTypeDeclarations()[0].accept(visitor.finder.comprehensiveVisitor());
+ /**
+ * Creates list of imports that are present in compilation unit.
+ * For example:
+ * [import io.netty.buffer.DrillBuf;, import org.apache.drill.exec.expr.DrillSimpleFunc;]
+ *
+ * @param compilationUnit compilation unit
+ * @return list of imports
+ */
+ public static List<String> getImports(Java.CompilationUnit compilationUnit){
--- End diff --
If the returned list was empty previously, what was the functionality previously?
And looks to me like `FunctionInitializer#imports` is unused. So should `ImportGrabber` be removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118184323
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -119,33 +117,35 @@ private void checkInit() {
}
}
- private CompilationUnit get(Class<?> c) throws IOException {
- String path = c.getName();
+ /**
+ * Using class name generates path to class source code (*.java),
+ * reads its content as string and parses it into {@link org.codehaus.janino.Java.CompilationUnit}.
+ *
+ * @param clazz function class
+ * @return compilation unit
+ * @throws IOException if did not find class or could not load it
+ */
+ public CompilationUnit convertToCompilationUnit(Class<?> clazz) throws IOException {
+ String path = clazz.getName();
path = path.replaceFirst("\\$.*", "");
path = path.replace(".", FileUtils.separator);
path = "/" + path + ".java";
- CompilationUnit cu = functionUnits.get(path);
- if (cu != null) {
- return cu;
- }
- try (InputStream is = c.getResourceAsStream(path)) {
+ logger.debug("Loading function code from the {}", path);
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #843: DRILL-5533: Fix flag assignment in FunctionInitializer.che...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/843
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #843: DRILL-5533: Fix flag assignment in FunctionInitializer.che...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/843
@sudheeshkatkam addressed all CR comments, please review. Thanks in advance!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118022413
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -119,33 +117,35 @@ private void checkInit() {
}
}
- private CompilationUnit get(Class<?> c) throws IOException {
- String path = c.getName();
+ /**
+ * Using class name generates path to class source code (*.java),
+ * reads its content as string and parses it into {@link org.codehaus.janino.Java.CompilationUnit}.
+ *
+ * @param clazz function class
+ * @return compilation unit
+ * @throws IOException if did not find class or could not load it
+ */
+ public CompilationUnit convertToCompilationUnit(Class<?> clazz) throws IOException {
+ String path = clazz.getName();
path = path.replaceFirst("\\$.*", "");
path = path.replace(".", FileUtils.separator);
path = "/" + path + ".java";
- CompilationUnit cu = functionUnits.get(path);
- if (cu != null) {
- return cu;
- }
- try (InputStream is = c.getResourceAsStream(path)) {
+ logger.debug("Loading function code from the {}", path);
--- End diff --
trace
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118021635
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -31,29 +31,25 @@
import org.codehaus.janino.Scanner;
import org.mortbay.util.IO;
-import com.google.common.collect.Maps;
-
/**
* To avoid the cost of initializing all functions up front,
- * this class contains all informations required to initializing a function when it is used.
+ * this class contains all information required to initializing a function when it is used.
*/
public class FunctionInitializer {
- static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
private final String className;
private final ClassLoader classLoader;
- private Map<String, CompilationUnit> functionUnits = Maps.newHashMap();
--- End diff --
Is this map unused?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118021745
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -74,41 +70,43 @@ public String getClassName() {
* @return the imports of this class (for java code gen)
*/
public List<String> getImports() {
- checkInit();
+ loadFunctionBody();
return imports;
}
/**
- * @param methodName
+ * @param methodName method name
* @return the content of the method (for java code gen inlining)
*/
public String getMethod(String methodName) {
- checkInit();
+ loadFunctionBody();
return methods.get(methodName);
}
- private void checkInit() {
- if (ready) {
+ /**
+ * Loads function body: methods (for instance, eval, setup, reset) and imports.
+ * Loading is done once per class instance upon first function invocation.
+ * Double-checked locking is used to avoid concurrency issues
+ * when two threads are trying to load the function body at the same time.
+ */
+ private void loadFunctionBody() {
+ if (isLoaded) {
return;
}
synchronized (this) {
- if (ready) {
+ if (isLoaded) {
return;
}
- // get function body.
-
+ logger.debug("Getting function body for the {}", className);
--- End diff --
trace
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/843#discussion_r118188385
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java ---
@@ -31,29 +31,25 @@
import org.codehaus.janino.Scanner;
import org.mortbay.util.IO;
-import com.google.common.collect.Maps;
-
/**
* To avoid the cost of initializing all functions up front,
- * this class contains all informations required to initializing a function when it is used.
+ * this class contains all information required to initializing a function when it is used.
*/
public class FunctionInitializer {
- static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
private final String className;
private final ClassLoader classLoader;
- private Map<String, CompilationUnit> functionUnits = Maps.newHashMap();
--- End diff --
It was used in `convertToCompilationUnit`. Before loading function body we checked if it was loaded before in `functionUnits` map. If not - function body was loaded, if yes - previously loaded compilation unit was returned. With incorrectly implemented DCL such check prevented us from loading function body from disk each time, through threads were entering synchronized block anyway. With correctly implemented DCL we don't need to store this intermediate result. First thread will load all information about methods and imports, others will use this information.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #843: DRILL-5533: Fix flag assignment in FunctionInitiali...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/843
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---