[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185507108
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/coord/zk/TestZookeeperClient.java
 ---
@@ -125,7 +125,7 @@ public void testHasPathThrowsDrillRuntimeException() {
 
 Mockito
 .when(client.getCache().getCurrentData(absPath))
-.thenThrow(Exception.class);
+.thenThrow(RuntimeException.class);
--- End diff --

Let's do it in different PR.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185497944
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/coord/zk/TestZookeeperClient.java
 ---
@@ -125,7 +125,7 @@ public void testHasPathThrowsDrillRuntimeException() {
 
 Mockito
 .when(client.getCache().getCurrentData(absPath))
-.thenThrow(Exception.class);
+.thenThrow(RuntimeException.class);
--- End diff --

IMO, this method needs to be changed to test 
`ZookeeperClient.hasPath(String path, boolean consistent)`. It is OK to do it 
in this PR or in a separate commit/JIRA/PR. If you decide to do it in a 
separate commit, please file JIRA.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185452673
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -35,41 +39,46 @@
 import org.apache.hadoop.fs.RemoteIterator;
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.junit.BeforeClass;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.rules.ExpectedException;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.util.List;
+import java.util.Optional;
 import java.util.Properties;
-import java.util.UUID;
 
 import static 
org.apache.drill.exec.util.StoragePluginTestUtils.DFS_PLUGIN_NAME;
 import static 
org.apache.drill.exec.util.StoragePluginTestUtils.DFS_TMP_SCHEMA;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 @Category(SqlTest.class)
 public class TestCTTAS extends BaseTestQuery {
 
-  private static final UUID session_id = 
UUID.nameUUIDFromBytes("sessionId".getBytes());
   private static final String temp2_wk = "tmp2";
   private static final String temp2_schema = String.format("%s.%s", 
DFS_PLUGIN_NAME, temp2_wk);
 
+  private static String sessionId;
   private static FileSystem fs;
   private static FsPermission expectedFolderPermission;
   private static FsPermission expectedFilePermission;
 
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
   @BeforeClass
   public static void init() throws Exception {
-MockUp uuidMockUp = mockRandomUUID(session_id);
--- End diff --

I had to re-write this part since `MockUp.tearDown()` is not present in 
newer jmockit version. Now we don't mock session id at all.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185456183
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -164,121 +166,113 @@ public void 
testResolveTemporaryTableWithPartialSchema() throws Exception {
   @Test
   public void testPartitionByWithTemporaryTables() throws Exception {
 String temporaryTableName = "temporary_table_with_partitions";
-mockRandomUUID(UUID.nameUUIDFromBytes(temporaryTableName.getBytes()));
+cleanSessionDirectory();
 test("create TEMPORARY table %s partition by (c1) as select * from (" +
 "select 'A' as c1 from (values(1)) union all select 'B' as c1 from 
(values(1))) t", temporaryTableName);
-checkPermission(temporaryTableName);
+checkPermission();
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreationOutsideOfDefaultTemporaryWorkspace() throws 
Exception {
-try {
-  String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
-  test("create TEMPORARY table %s.%s as select 'A' as c1 from 
(values(1))", temp2_schema, temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: Temporary tables are not allowed to be 
created / dropped " +
-  "outside of default temporary workspace [%s].", 
DFS_TMP_SCHEMA)));
-  throw e;
-}
+String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: Temporary tables are not allowed to be created 
/ dropped " +
+"outside of default temporary workspace [%s].", 
DFS_TMP_SCHEMA)));
+
+test("create TEMPORARY table %s.%s as select 'A' as c1 from 
(values(1))", temp2_schema, temporaryTableName);
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreateWhenTemporaryTableExistsWithoutSchema() throws 
Exception {
 String temporaryTableName = "temporary_table_exists_without_schema";
-try {
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
- "VALIDATION ERROR: A table or view with given name [%s]" +
- " already exists in schema [%s]", temporaryTableName, 
DFS_TMP_SCHEMA)));
-  throw e;
-}
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: A table or view with given name [%s]" +
+" already exists in schema [%s]", temporaryTableName, 
DFS_TMP_SCHEMA)));
+
+test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
+test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreateWhenTemporaryTableExistsCaseInsensitive() throws 
Exception {
 String temporaryTableName = "temporary_table_exists_without_schema";
-try {
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName.toUpperCase());
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: A table or view with given name [%s]" +
-  " already exists in schema [%s]", 
temporaryTableName.toUpperCase(), DFS_TMP_SCHEMA)));
-  throw e;
-}
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: A table or view with given name [%s]" +
--- End diff --

Done.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185449640
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/coord/zk/TestZookeeperClient.java
 ---
@@ -125,7 +125,7 @@ public void testHasPathThrowsDrillRuntimeException() {
 
 Mockito
 .when(client.getCache().getCurrentData(absPath))
-.thenThrow(Exception.class);
+.thenThrow(RuntimeException.class);
--- End diff --

I just fixed this test to work with new mockito lib version. I suspect this 
test checks if exception thrown from 
`client.getCache().getCurrentData(absPath)` method will be wrapped in 
DrillRuntimeException.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185450679
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java 
---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.udf.dynamic;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.ConsoleAppender;
+import org.apache.maven.cli.MavenCli;
+import org.apache.maven.cli.logging.Slf4jLogger;
+import org.codehaus.plexus.DefaultPlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.logging.BaseLoggerManager;
+import org.slf4j.LoggerFactory;
+
+import java.util.LinkedList;
+import java.util.List;
+
+public class JarBuilder {
+
+  private final MavenCli cli;
+
+  public JarBuilder() {
+this.cli = new MavenCli() {
+  @Override
+  protected void customizeContainer(PlexusContainer container) {
+((DefaultPlexusContainer) container).setLoggerManager(new 
BaseLoggerManager() {
+  @Override
+  protected org.codehaus.plexus.logging.Logger createLogger(String 
s) {
+return new Slf4jLogger(setupLogger(JarBuilder.class.getName(), 
Level.INFO));
+  }
+});
+  }
+};
+  }
+
+  /**
+   * Builds jars using embedded maven. Includes files / resources based 
given pattern,
+   * otherwise using defaults provided in pom.xml.
+   *
+   * @param jarName jar name
+   * @param projectDir project dir
+   * @param includeFiles pattern indicating which files should be included
+   * @param includeResources pattern indicating which resources should be 
included
+   *
+   * @return build exit code, 0 if build was successful
+   */
+  public int build(String jarName, String projectDir, String includeFiles, 
String includeResources) {
+System.setProperty("maven.multiModuleProjectDirectory", projectDir);
+List params = new LinkedList<>();
+params.add("clean");
+params.add("package");
+params.add("-DskipTests");
+params.add("-Djar.finalName=" + jarName);
+if (includeFiles != null) {
+  params.add("-Dinclude.files=" + includeFiles);
+}
+if (includeResources != null) {
+  params.add("-Dinclude.resources=" + includeResources);
+}
+return cli.doMain(params.toArray(new String[params.size()]), 
projectDir, System.out, System.err);
+  }
+
+  private static Logger setupLogger(String string, Level logLevel) {
--- End diff --

Yes, since I want to output info level logging when creating jars. 
Otherwise, no logs on how jars were created will appear.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185450442
  
--- Diff: exec/java-exec/src/test/resources/drill-udf/pom.xml ---
@@ -0,0 +1,90 @@
+
+
+http://maven.apache.org/POM/4.0.0;
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+  4.0.0
+
+  org.apache.drill.udf
+  drill-udf
+  1.0
+
+  
+${project.name}
+1.13.0
--- End diff --

My previous jars used 1.8.0. for example, since I created them when that 
only version was available. API for Drill UDFs is public so we should not 
change it. There is no enforcement though. 


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185452897
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -498,47 +489,50 @@ public void 
testDropTemporaryTableAsViewWithoutException() throws Exception {
 .go();
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testDropTemporaryTableAsViewWithException() throws Exception 
{
 String temporaryTableName = 
"temporary_table_to_drop_like_view_with_exception";
 test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
 
-try {
-  test("drop view %s.%s", DFS_TMP_SCHEMA, temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: Unknown view [%s] in schema [%s]", 
temporaryTableName, DFS_TMP_SCHEMA)));
-  throw e;
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: Unknown view [%s] in schema [%s]", 
temporaryTableName, DFS_TMP_SCHEMA)));
+
+test("drop view %s.%s", DFS_TMP_SCHEMA, temporaryTableName);
+  }
+
+  private static String getSessionId() throws Exception {
--- End diff --

Re-written, it turned out I have access to `UserSession` through 
`DrillbitContext`, so no mock is required at all.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185383432
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java 
---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.udf.dynamic;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.ConsoleAppender;
+import org.apache.maven.cli.MavenCli;
+import org.apache.maven.cli.logging.Slf4jLogger;
+import org.codehaus.plexus.DefaultPlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.logging.BaseLoggerManager;
+import org.slf4j.LoggerFactory;
+
+import java.util.LinkedList;
+import java.util.List;
+
+public class JarBuilder {
+
+  private final MavenCli cli;
+
+  public JarBuilder() {
+this.cli = new MavenCli() {
+  @Override
+  protected void customizeContainer(PlexusContainer container) {
+((DefaultPlexusContainer) container).setLoggerManager(new 
BaseLoggerManager() {
+  @Override
+  protected org.codehaus.plexus.logging.Logger createLogger(String 
s) {
+return new Slf4jLogger(setupLogger(JarBuilder.class.getName(), 
Level.INFO));
+  }
+});
+  }
+};
+  }
+
+  /**
+   * Builds jars using embedded maven. Includes files / resources based 
given pattern,
+   * otherwise using defaults provided in pom.xml.
+   *
+   * @param jarName jar name
+   * @param projectDir project dir
+   * @param includeFiles pattern indicating which files should be included
+   * @param includeResources pattern indicating which resources should be 
included
+   *
+   * @return build exit code, 0 if build was successful
+   */
+  public int build(String jarName, String projectDir, String includeFiles, 
String includeResources) {
+System.setProperty("maven.multiModuleProjectDirectory", projectDir);
+List params = new LinkedList<>();
+params.add("clean");
+params.add("package");
+params.add("-DskipTests");
+params.add("-Djar.finalName=" + jarName);
+if (includeFiles != null) {
+  params.add("-Dinclude.files=" + includeFiles);
+}
+if (includeResources != null) {
+  params.add("-Dinclude.resources=" + includeResources);
+}
+return cli.doMain(params.toArray(new String[params.size()]), 
projectDir, System.out, System.err);
+  }
+
+  private static Logger setupLogger(String string, Level logLevel) {
--- End diff --

Is this necessary?


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185385028
  
--- Diff: exec/java-exec/src/test/resources/drill-udf/pom.xml ---
@@ -0,0 +1,90 @@
+
+
+http://maven.apache.org/POM/4.0.0;
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+  4.0.0
+
+  org.apache.drill.udf
+  drill-udf
+  1.0
+
+  
+${project.name}
+1.13.0
--- End diff --

Is it OK to use old version? Does Drill support semver API compatibility 
for UDFs? If yes, how is it enforced? If no, compilation may fail.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185353418
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java
 ---
@@ -177,7 +177,7 @@ public void run() {
 }
   }
 
-  //@Test
--- End diff --

What is the reason the test was disabled before?


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185374827
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -164,121 +166,113 @@ public void 
testResolveTemporaryTableWithPartialSchema() throws Exception {
   @Test
   public void testPartitionByWithTemporaryTables() throws Exception {
 String temporaryTableName = "temporary_table_with_partitions";
-mockRandomUUID(UUID.nameUUIDFromBytes(temporaryTableName.getBytes()));
+cleanSessionDirectory();
 test("create TEMPORARY table %s partition by (c1) as select * from (" +
 "select 'A' as c1 from (values(1)) union all select 'B' as c1 from 
(values(1))) t", temporaryTableName);
-checkPermission(temporaryTableName);
+checkPermission();
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreationOutsideOfDefaultTemporaryWorkspace() throws 
Exception {
-try {
-  String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
-  test("create TEMPORARY table %s.%s as select 'A' as c1 from 
(values(1))", temp2_schema, temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: Temporary tables are not allowed to be 
created / dropped " +
-  "outside of default temporary workspace [%s].", 
DFS_TMP_SCHEMA)));
-  throw e;
-}
+String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: Temporary tables are not allowed to be created 
/ dropped " +
+"outside of default temporary workspace [%s].", 
DFS_TMP_SCHEMA)));
+
+test("create TEMPORARY table %s.%s as select 'A' as c1 from 
(values(1))", temp2_schema, temporaryTableName);
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreateWhenTemporaryTableExistsWithoutSchema() throws 
Exception {
 String temporaryTableName = "temporary_table_exists_without_schema";
-try {
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
- "VALIDATION ERROR: A table or view with given name [%s]" +
- " already exists in schema [%s]", temporaryTableName, 
DFS_TMP_SCHEMA)));
-  throw e;
-}
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: A table or view with given name [%s]" +
+" already exists in schema [%s]", temporaryTableName, 
DFS_TMP_SCHEMA)));
+
+test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
+test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreateWhenTemporaryTableExistsCaseInsensitive() throws 
Exception {
 String temporaryTableName = "temporary_table_exists_without_schema";
-try {
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName);
-  test("create TEMPORARY table %s as select 'A' as c1 from 
(values(1))", temporaryTableName.toUpperCase());
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: A table or view with given name [%s]" +
-  " already exists in schema [%s]", 
temporaryTableName.toUpperCase(), DFS_TMP_SCHEMA)));
-  throw e;
-}
+
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: A table or view with given name [%s]" +
--- End diff --

and possibly `expectUserRemoteExceptionWithTableExistsMessage(String 
tableName, String schemaName)`.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185375540
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -498,47 +489,50 @@ public void 
testDropTemporaryTableAsViewWithoutException() throws Exception {
 .go();
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testDropTemporaryTableAsViewWithException() throws Exception 
{
 String temporaryTableName = 
"temporary_table_to_drop_like_view_with_exception";
 test("create TEMPORARY table %s as select 'A' as c1 from (values(1))", 
temporaryTableName);
 
-try {
-  test("drop view %s.%s", DFS_TMP_SCHEMA, temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: Unknown view [%s] in schema [%s]", 
temporaryTableName, DFS_TMP_SCHEMA)));
-  throw e;
+thrown.expect(UserRemoteException.class);
+thrown.expectMessage(containsString(String.format(
+"VALIDATION ERROR: Unknown view [%s] in schema [%s]", 
temporaryTableName, DFS_TMP_SCHEMA)));
+
+test("drop view %s.%s", DFS_TMP_SCHEMA, temporaryTableName);
+  }
+
+  private static String getSessionId() throws Exception {
--- End diff --

Consider mocking getSessionId() in the `UserSession`. This method needs to 
be tested by itself.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185369981
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/coord/zk/TestZookeeperClient.java
 ---
@@ -125,7 +125,7 @@ public void testHasPathThrowsDrillRuntimeException() {
 
 Mockito
 .when(client.getCache().getCurrentData(absPath))
-.thenThrow(Exception.class);
+.thenThrow(RuntimeException.class);
--- End diff --

OK, but I am not sure what does this method test. 
`ZookeeperClient.hasPath(String path)` is not used in production.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r185374205
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java ---
@@ -164,121 +166,113 @@ public void 
testResolveTemporaryTableWithPartialSchema() throws Exception {
   @Test
   public void testPartitionByWithTemporaryTables() throws Exception {
 String temporaryTableName = "temporary_table_with_partitions";
-mockRandomUUID(UUID.nameUUIDFromBytes(temporaryTableName.getBytes()));
+cleanSessionDirectory();
 test("create TEMPORARY table %s partition by (c1) as select * from (" +
 "select 'A' as c1 from (values(1)) union all select 'B' as c1 from 
(values(1))) t", temporaryTableName);
-checkPermission(temporaryTableName);
+checkPermission();
   }
 
-  @Test(expected = UserRemoteException.class)
+  @Test
   public void testCreationOutsideOfDefaultTemporaryWorkspace() throws 
Exception {
-try {
-  String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
-  test("create TEMPORARY table %s.%s as select 'A' as c1 from 
(values(1))", temp2_schema, temporaryTableName);
-} catch (UserRemoteException e) {
-  assertThat(e.getMessage(), containsString(String.format(
-  "VALIDATION ERROR: Temporary tables are not allowed to be 
created / dropped " +
-  "outside of default temporary workspace [%s].", 
DFS_TMP_SCHEMA)));
-  throw e;
-}
+String temporaryTableName = 
"temporary_table_outside_of_default_workspace";
+
+thrown.expect(UserRemoteException.class);
--- End diff --

Consider introducing a new method to set `thrown` and message, something 
like `void expectUserRemoteExceptionWithMessage(String message)`.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r184411700
  
--- Diff: exec/java-exec/pom.xml ---
@@ -593,6 +593,48 @@
   netty-tcnative
   ${netty.tcnative.classifier}
 
+
+  org.apache.maven
+  maven-embedder
+  3.3.9
--- End diff --

Consider using the latest release available.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r184245358
  
--- Diff: pom.xml ---
@@ -798,7 +798,7 @@
   
   com.googlecode.jmockit
   jmockit
-  1.3
+  1.7
--- End diff --

Can it be done as a precursor PR? 1.7 version is quite old too. Can it be 
upgraded to the latest (org.jmockit:jmockit:1.39)?


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r182902596
  
--- Diff: 
exec/java-exec/src/test/resources/udf/dynamic/CustomAbsFunctionTemplate ---
@@ -0,0 +1,45 @@
+package org.apache.drill.udf.dynamic;
--- End diff --

Apache license


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r182902723
  
--- Diff: 
exec/java-exec/src/test/resources/udf/dynamic/CustomAbsFunctionTemplate ---
@@ -0,0 +1,45 @@
+package org.apache.drill.udf.dynamic;
+
+import io.netty.buffer.DrillBuf;
+import org.apache.drill.exec.expr.DrillSimpleFunc;
+import org.apache.drill.exec.expr.annotations.FunctionTemplate;
+import org.apache.drill.exec.expr.annotations.Output;
+import org.apache.drill.exec.expr.annotations.Param;
+import org.apache.drill.exec.expr.holders.VarCharHolder;
+
+import javax.inject.Inject;
+
+@FunctionTemplate(
+name="abs",
+scope= FunctionTemplate.FunctionScope.SIMPLE,
+nulls = FunctionTemplate.NullHandling.NULL_IF_NULL
+)
+public class Abs implements DrillSimpleFunc {
+
+  @Param
+  VarCharHolder input1;
+
+  @Param
+  VarCharHolder input2;
+
+  @Output
+  VarCharHolder out;
+
+  @Inject
+  DrillBuf buffer;
+
+  public void setup() {
+
+  }
+
+  public void eval() {
+String inputString1 = 
org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.toStringFromUTF8(input1.start,
 input1.end, input1.buffer);
+String inputString2 = 
org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.toStringFromUTF8(input2.start,
 input2.end, input2.buffer);
+String outputValue = String.format("ABS was overloaded. Input: %s, 
%s", inputString1, inputString2);
+
+out.buffer = buffer;
+out.start = 0;
+out.end = outputValue.getBytes().length;
+buffer.setBytes(0, outputValue.getBytes());
+  }
+}
--- End diff --

LF


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1225#discussion_r182902184
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java
 ---
@@ -53,17 +56,27 @@
 @Category(SqlFunctionTest.class)
 public class FunctionInitializerTest {
 
-  private static final String CLASS_NAME = 
"com.drill.udf.CustomLowerFunction";
+  private static final String CLASS_NAME = 
"org.apache.drill.udf.dynamic.CustomLowerFunction";
   private static URLClassLoader classLoader;
 
+  @ClassRule
+  public static TemporaryFolder temporaryFolder = new TemporaryFolder();
+
   @BeforeClass
   public static void init() throws Exception {
-Path jars = TestTools.WORKING_PATH
-  .resolve(TestTools.TEST_RESOURCES)
-  .resolve("jars");
-String binaryName = "DrillUDF-1.0.jar";
-String sourceName = JarUtil.getSourceName(binaryName);
-URL[] urls = {jars.resolve(binaryName).toUri().toURL(), 
jars.resolve(sourceName).toUri().toURL()};
+String binaryName = "DrillUDF-1.0";
+URL template = 
ClassLoader.getSystemClassLoader().getResource("udf/dynamic/CustomLowerFunctionTemplate");
+assert template != null;
--- End diff --

use junit assert in unit tests.


---

[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...

[arina-ielchiieva]

GitHub user arina-ielchiieva opened a pull request:

https://github.com/apache/drill/pull/1225

DRILL-6272: Refactor dynamic UDFs and function initializer tests to g…

…enerate needed binary and source jars at runtime

Details in [DRILL-6272](https://issues.apache.org/jira/browse/DRILL-6272).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/arina-ielchiieva/drill DRILL-6272

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1225.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 #1225


commit b31ffa39330c4e39531132a2abec1dab17fb97b0
Author: Arina Ielchiieva 
Date:   2018-04-05T15:33:55Z

DRILL-6272: Refactor dynamic UDFs and function initializer tests to 
generate needed binary and source jars at runtime




---