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 2018/04/19 17:45:30 UTC
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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 <ar...@...>
Date: 2018-04-05T15:33:55Z
DRILL-6272: Refactor dynamic UDFs and function initializer tests to generate needed binary and source jars at runtime
----
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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/1225#discussion_r185459187
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java ---
@@ -43,27 +44,32 @@
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicInteger;
+import static org.apache.drill.test.TestTools.getResourceFile;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.spy;
-@RunWith(MockitoJUnitRunner.class)
@Category(SqlFunctionTest.class)
public class FunctionInitializerTest {
- private static final String CLASS_NAME = "com.drill.udf.CustomLowerFunction";
+ @ClassRule
+ public static final BaseDirTestWatcher dirTestWatcher = new BaseDirTestWatcher();
+
+ private static final String CLASS_NAME = "org.apache.drill.udf.dynamic.CustomLowerFunction";
private static URLClassLoader classLoader;
@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()};
+ File projectDir = dirTestWatcher.makeSubDir(Paths.get("drill-udf"));
+ FileUtils.copyDirectory(getResourceFile(Paths.get(projectDir.getName())), projectDir);
--- End diff --
Done, thanks.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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/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...
Posted by vrozov <gi...@git.apache.org>.
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...
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/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<String> 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...
Posted by vrozov <gi...@git.apache.org>.
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 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="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">
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.drill.udf</groupId>
+ <artifactId>drill-udf</artifactId>
+ <version>1.0</version>
+
+ <properties>
+ <jar.finalName>${project.name}</jar.finalName>
+ <drill.version>1.13.0</drill.version>
--- 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...
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/1225#discussion_r185449014
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java ---
@@ -177,7 +177,7 @@ public void run() {
}
}
- //@Test
--- End diff --
I have accidentally disabled in my previous PR. Did not want to create separate Jira for one line fix so included it here.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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/1225#discussion_r185456207
--- 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 --
Done.
---
[GitHub] drill issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1225
@arina-ielchiieva I don't see why using [maven embedder](http://maven.apache.org/ref/3.5.2/maven-embedder) is a less preferable option. Using maven it is possible to create source jars using standard maven plugin. IMO, it will be easier to modify test UDF project if necessary and it is still possible to debug and run tests using IDE or maven. There will be no dependency on a build tool, it is a dependency on an external jar similar to any other external jar dependency.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
Posted by vrozov <gi...@git.apache.org>.
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<String> 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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1225#discussion_r185373456
--- 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<UUID> uuidMockUp = mockRandomUUID(session_id);
--- End diff --
It is hard to say whether you want to change test logic or need a workaround for the removed `MockUp.tearDown()` method. In the latter case consider adding `getTemporaryName()` method to the `UserSession` class. The method can delegate to `UUID.randomUUID()` in production and can be mocked to return a predefined value in tests like this one. It will allow not to mock `UUID.randomUUID()` at all.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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/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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1225#discussion_r185384630
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java ---
@@ -43,27 +44,32 @@
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicInteger;
+import static org.apache.drill.test.TestTools.getResourceFile;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.spy;
-@RunWith(MockitoJUnitRunner.class)
@Category(SqlFunctionTest.class)
public class FunctionInitializerTest {
- private static final String CLASS_NAME = "com.drill.udf.CustomLowerFunction";
+ @ClassRule
+ public static final BaseDirTestWatcher dirTestWatcher = new BaseDirTestWatcher();
+
+ private static final String CLASS_NAME = "org.apache.drill.udf.dynamic.CustomLowerFunction";
private static URLClassLoader classLoader;
@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()};
+ File projectDir = dirTestWatcher.makeSubDir(Paths.get("drill-udf"));
+ FileUtils.copyDirectory(getResourceFile(Paths.get(projectDir.getName())), projectDir);
--- End diff --
Consider using `<directory>` in pom.xml if the goal here is to avoid creating files in the `src` path.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
Posted by vrozov <gi...@git.apache.org>.
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 issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov please review.
---
[GitHub] drill issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov addressed code review comments in the new commit. Please review.
---
[GitHub] drill issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov now PR contains two commits:
1. jmockit and mockito upgrade (DRILL-6363);
2. maven-embedder usage for unit tests (used latest version as you suggested) (DRILL-6272).
Please review.
---
[GitHub] drill issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov addressed code review comments.
Regarding embedded maven, I have considered this option but using native Java to generate jars is more convenient. First of all, for the tests we need different jars, with and without resource file, classes. Doing it in Java allows to store less config and template files. Also we do not depend from any build tool. Plus it much easier to debug and run test using IDE or maven. I suggest we leave the current approach.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
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/1225#discussion_r185450442
--- Diff: exec/java-exec/src/test/resources/drill-udf/pom.xml ---
@@ -0,0 +1,90 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<project xmlns="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">
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.drill.udf</groupId>
+ <artifactId>drill-udf</artifactId>
+ <version>1.0</version>
+
+ <properties>
+ <jar.finalName>${project.name}</jar.finalName>
+ <drill.version>1.13.0</drill.version>
--- 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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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 @@
<!-- JMockit needs to be on class path before JUnit. -->
<groupId>com.googlecode.jmockit</groupId>
<artifactId>jmockit</artifactId>
- <version>1.3</version>
+ <version>1.7</version>
--- 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...
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/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...
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/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...
Posted by vrozov <gi...@git.apache.org>.
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 @@
<artifactId>netty-tcnative</artifactId>
<classifier>${netty.tcnative.classifier}</classifier>
</dependency>
+ <dependency>
+ <groupId>org.apache.maven</groupId>
+ <artifactId>maven-embedder</artifactId>
+ <version>3.3.9</version>
--- End diff --
Consider using the latest release available.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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 issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov I'll re-work PR with maven embedder, thanks for the idea. I'll ping you when changes are done.
---
[GitHub] drill pull request #1225: DRILL-6272: Refactor dynamic UDFs and function ini...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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...
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/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<UUID> 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 issue #1225: DRILL-6272: Refactor dynamic UDFs and function initialize...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1225
@vrozov re-implemented using maven embedder. Had to upgrade jmokcit lib to the latest version since it caused NPE with maven embedder (NPE from jmockit even if no mocks are used, fixed in newer version). Please review.
---