You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by paul-rogers <gi...@git.apache.org> on 2017/03/19 03:12:41 UTC
[GitHub] drill pull request #788: DRILL-5318: Sub-operator test fixture
GitHub user paul-rogers opened a pull request:
https://github.com/apache/drill/pull/788
DRILL-5318: Sub-operator test fixture
This commit depends on:
* DRILL-5319
* DRILL-5323
* DRILL-5324
This PR cannot be accepted (or built) until the above are pulled and
this PR is rebased on top of them. The PR is issued now so that reviews
can be done in parallel.
Provides the following:
* A new OperatorFixture to set up all the objects needed to test at the
sub-operator level. This relies on the refactoring to create the
required interfaces.
* Pulls the config builder code out of the cluster fixture builder so
that configs can be build for sub-operator tests.
* Modifies the QueryBuilder test tool to run a query and get back one
of the new row set objects to allow direct inspection of data returned
from a query.
* Modifies the cluster fixture to create a JDBC connection to the test
cluster. (Use requires putting the Drill JDBC project on the test class
path since exec does not depend on JDBC.)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paul-rogers/drill DRILL-5318
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/788.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 #788
----
commit 279baaf3eda0bf669f76e217b1fd978f2e46f572
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-03-19T03:12:17Z
DRILL-5318: Sub-operator test fixture
This commit depends on:
* DRILL-5319
* DRILL-5323
* DRILL-5324
This PR cannot be accepted (or built) until the above are pulled and
this PR is rebased on top of them. The PR is issued now so that reviews
can be done in parallel.
Provides the following:
* A new OperatorFixture to set up all the objects needed to test at the
sub-operator level. This relies on the refactoring to create the
required interfaces.
* Pulls the config builder code out of the cluster fixture builder so
that configs can be build for sub-operator tests.
* Modifies the QueryBuilder test tool to run a query and get back one
of the new row set objects to allow direct inspection of data returned
from a query.
* Modifies the cluster fixture to create a JDBC connection to the test
cluster. (Use requires putting the Drill JDBC project on the test class
path since exec does not depend on JDBC.)
----
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r113084244
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -0,0 +1,400 @@
+/*
+ * 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.test.rowSet.test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.drill.common.types.TypeProtos.DataMode;
+import org.apache.drill.common.types.TypeProtos.MinorType;
+import org.apache.drill.exec.record.BatchSchema;
+import org.apache.drill.exec.vector.accessor.ArrayReader;
+import org.apache.drill.exec.vector.accessor.ArrayWriter;
+import org.apache.drill.exec.vector.accessor.TupleAccessor.TupleSchema;
+import org.apache.drill.test.OperatorFixture;
+import org.apache.drill.test.rowSet.RowSet.ExtendableRowSet;
+import org.apache.drill.test.rowSet.RowSet.RowSetReader;
+import org.apache.drill.test.rowSet.RowSet.RowSetWriter;
+import org.apache.drill.test.rowSet.RowSet.SingleRowSet;
+import org.apache.drill.test.rowSet.RowSetComparison;
+import org.apache.drill.test.rowSet.RowSetSchema;
+import org.apache.drill.test.rowSet.RowSetSchema.FlattenedSchema;
+import org.apache.drill.test.rowSet.RowSetSchema.PhysicalSchema;
+import org.apache.drill.test.rowSet.SchemaBuilder;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.google.common.base.Splitter;
+
+public class RowSetTest {
+
+ private static OperatorFixture fixture;
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ fixture = OperatorFixture.standardFixture();
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
+ fixture.close();
--- End diff --
Is this the only unit test added to DRILL-5318 / DRILL-5323 which are for unit test framework related enhancements? Looks like the new tests in this class are to test the new added concept such as "RowSet", "RowSetSchema", "FlattenedSchema". Do we have some example that shows how to use those concepts to do a operator / sub-operator testing, which I thought are the main purpose of the two PRs?
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r108094926
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/ConfigBuilder.java ---
@@ -0,0 +1,130 @@
+/*
+ * 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.test;
+
+import java.util.Properties;
+import java.util.Map.Entry;
+
+import org.apache.drill.common.config.DrillConfig;
+
+/**
+ * Builds a {@link DrillConfig} for use in tests. Use this when a config
+ * is needed by itself, separate from an embedded Drillbit.
+ */
+public class ConfigBuilder {
+
+ protected String configResource;
+ protected Properties configProps;
+
+ /**
+ * Use the given configuration properties as overrides.
+ * @param configProps a collection of config properties
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder configProps(Properties configProps) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (this.configProps == null) {
+ this.configProps = configProps;
+ } else {
+ this.configProps.putAll(configProps);
+ }
+ return this;
+ }
+
+ /**
+ * Use the given configuration file, stored as a resource, to initialize
+ * the Drill config. Note that the resource file should have the two
+ * following settings to work as a config for an embedded Drillbit:
+ * <pre><code>
+ * drill.exec.sys.store.provider.local.write : false,
+ * drill.exec.http.enabled : false
+ * </code></pre>
+ * It may be more convenient to add your settings to the default
+ * config settings with {@link #configProperty(String, Object)}.
+ * @param configResource path to the file that contains the
+ * config file to be read
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder resource(String configResource) {
+
+ if (configProps != null) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+
+ // TypeSafe gets unhappy about a leading slash, but other functions
+ // require it. Silently discard the leading slash if given to
+ // preserve the test writer's sanity.
+
+ this.configResource = ClusterFixture.trimSlash(configResource);
+ return this;
+ }
+
+ /**
+ * Add an additional boot-time property for the embedded Drillbit.
+ * @param key config property name
+ * @param value property value
+ * @return this builder
+ */
+
+ public ConfigBuilder put(String key, Object value) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (configProps == null) {
+ configProps = new Properties();
+ }
+ configProps.put(key, value.toString());
+ return this;
+ }
+
+ public DrillConfig build() {
+
+ // Create a config
+ // Because of the way DrillConfig works, we can set the ZK
+ // connection string only if a property set is provided.
+
+ if (hasResource()) {
+ return DrillConfig.create(configResource);
+ } else if (configProps != null) {
+ return DrillConfig.create(configProperties());
+ } else {
+ return DrillConfig.create();
+ }
+ }
+
+ private Properties configProperties() {
--- End diff --
This function is just returning original configProps. Do we need to create a new Properties object here ?
As per previous implementation it was returning the properties based on input.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/788
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/788
One of the key reasons for creating tests is that they show how to use an API. In this case, the associated tests show how to use the row set abstractions to create a schema, populate a row set and so on. Please also see the package-info.java file for Javadoc that provides an overview.
Please let me know if you want to use the test framework. If so, I'd like to work with you to figure out what additional documentation and/or examples would be useful.
This PR is a dependency on the "main show": a PR with refactored sort code and complete sort unit tests. That PR will be submitted this week now that all the prerequisites are in master. That will demonstrate now to use the framework to create a suite of unit tests. I can point to my personal repo if if it would be helpful to preview that code.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/788
This does not compile. Please fix the errors and squash the commits into one.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/788
I am confused by your comment. Which should be committed first, 5318 or 5323? Your comments says 5318, but the commit message for 5318 and the fact that 5318 is based on 5323 suggests 5323 should be committed first?
Please format the commit message in 70fdc88 so it starts with "DRILL-XXXX: ..."
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r109539506
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/QueryBuilder.java ---
@@ -271,6 +276,91 @@ public QuerySummary run() throws Exception {
}
/**
+ * Run the query and return the first result set as a
+ * {@link DirectRowSet} object that can be inspected directly
+ * by the code using a {@link RowSetReader}.
+ * <p>
+ * An enhancement is to provide a way to read a series of result
+ * batches as row sets.
+ * @return a row set that represents the first batch returned from
+ * the query
+ * @throws RpcException if anything goes wrong
+ */
+
+ public DirectRowSet rowSet() throws RpcException {
+
+ // Ignore all but the first non-empty batch.
+
+ QueryDataBatch dataBatch = null;
+ for (QueryDataBatch batch : results()) {
+ if (dataBatch == null && batch.getHeader().getRowCount() != 0) {
+ dataBatch = batch;
--- End diff --
The comment says, "Ignore all but the first non-empty batch." We detect if a batch is non-empty by checking the row count. We detect if it is the first such batch by checking if we already have our desired batch. The reason for the funny code is that we want to release memory for all but the first batch.
The reason we get only the first batch is that this bit of test code only makes sense for small result sets; one would not use this for, say, 10K records. So, the bit about non-first batches is just for hygiene, no nasty memory complaints if you happen to return more than one batch.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r109541450
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/QueryBuilder.java ---
@@ -271,6 +276,91 @@ public QuerySummary run() throws Exception {
}
/**
+ * Run the query and return the first result set as a
+ * {@link DirectRowSet} object that can be inspected directly
+ * by the code using a {@link RowSetReader}.
+ * <p>
+ * An enhancement is to provide a way to read a series of result
+ * batches as row sets.
+ * @return a row set that represents the first batch returned from
+ * the query
+ * @throws RpcException if anything goes wrong
+ */
+
+ public DirectRowSet rowSet() throws RpcException {
+
+ // Ignore all but the first non-empty batch.
+
+ QueryDataBatch dataBatch = null;
+ for (QueryDataBatch batch : results()) {
+ if (dataBatch == null && batch.getHeader().getRowCount() != 0) {
+ dataBatch = batch;
+ } else {
+ batch.release();
+ }
+ }
+
+ // No results?
+
+ if (dataBatch == null) {
+ return null;
+ }
+
+ // Unload the batch and convert to a row set.
+
+ final RecordBatchLoader loader = new RecordBatchLoader(client.allocator());
+ try {
+ loader.load(dataBatch.getHeader().getDef(), dataBatch.getData());
+ dataBatch.release();
+ VectorContainer container = loader.getContainer();
+ container.setRecordCount(loader.getRecordCount());
+ return new DirectRowSet(client.allocator(), container);
+ } catch (SchemaChangeException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ /**
+ * Run the query that is expected to return (at least) one row
+ * with the only (or first) column returning a long value.
+ *
+ * @return the value of the first column of the first row
+ * @throws RpcException if anything goes wrong
+ */
+
+ public long singletonLong() throws RpcException {
+ RowSet rowSet = rowSet();
+ if (rowSet == null) {
+ throw new IllegalStateException("No rows returned");
+ }
+ RowSetReader reader = rowSet.reader();
+ reader.next();
+ long value = reader.column(0).getLong();
--- End diff --
Refactored some. To reuse all code, I'd need to create a class so we can grab the typed value in the middle of the other stuff. The few lines of repetition are the price of avoiding a trivial class. Since this is test code, I'm inclined to live with the (reduced) repetition. Take a look and let me know what you think. Also added a `singletonString()` method (because, why not?) and a new `reader()` method with some of the common code.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/788
Revised the commit header. Note that the commit messages also identifies that this PR is dependent on DRILL-5323, and so this PR must be committed after DRILL-5323.
Sorry for the confusion. The work was broken into multiple commits to ease the burden on code reviewers. Unfortunately, there is no free lunch, and the multiple commits puts a large burden on the submitter to keep all branches in sync, and on the committer to pull the PRs in the correct order.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r109538974
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/ConfigBuilder.java ---
@@ -0,0 +1,130 @@
+/*
+ * 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.test;
+
+import java.util.Properties;
+import java.util.Map.Entry;
+
+import org.apache.drill.common.config.DrillConfig;
+
+/**
+ * Builds a {@link DrillConfig} for use in tests. Use this when a config
+ * is needed by itself, separate from an embedded Drillbit.
+ */
+public class ConfigBuilder {
+
+ protected String configResource;
+ protected Properties configProps;
+
+ /**
+ * Use the given configuration properties as overrides.
+ * @param configProps a collection of config properties
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder configProps(Properties configProps) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (this.configProps == null) {
+ this.configProps = configProps;
+ } else {
+ this.configProps.putAll(configProps);
+ }
+ return this;
+ }
+
+ /**
+ * Use the given configuration file, stored as a resource, to initialize
+ * the Drill config. Note that the resource file should have the two
+ * following settings to work as a config for an embedded Drillbit:
+ * <pre><code>
+ * drill.exec.sys.store.provider.local.write : false,
+ * drill.exec.http.enabled : false
+ * </code></pre>
+ * It may be more convenient to add your settings to the default
+ * config settings with {@link #configProperty(String, Object)}.
+ * @param configResource path to the file that contains the
+ * config file to be read
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder resource(String configResource) {
+
+ if (configProps != null) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+
+ // TypeSafe gets unhappy about a leading slash, but other functions
+ // require it. Silently discard the leading slash if given to
+ // preserve the test writer's sanity.
+
+ this.configResource = ClusterFixture.trimSlash(configResource);
+ return this;
+ }
+
+ /**
+ * Add an additional boot-time property for the embedded Drillbit.
+ * @param key config property name
+ * @param value property value
+ * @return this builder
+ */
+
+ public ConfigBuilder put(String key, Object value) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (configProps == null) {
+ configProps = new Properties();
+ }
+ configProps.put(key, value.toString());
+ return this;
+ }
+
+ public DrillConfig build() {
+
+ // Create a config
+ // Because of the way DrillConfig works, we can set the ZK
+ // connection string only if a property set is provided.
+
+ if (hasResource()) {
+ return DrillConfig.create(configResource);
+ } else if (configProps != null) {
+ return DrillConfig.create(configProperties());
+ } else {
+ return DrillConfig.create();
+ }
+ }
+
+ private Properties configProperties() {
--- End diff --
Here is what should be happening, let me know if you see something different.
For convenience, the config builder lets you specify property values as typed values:
```
builder.put("some.boolean", true);
builder.put("some.int", 10);
builder.put("some.string", "foo");
```
The TypeSafe config system wants properties, but they must all be in the form of strings. For the above, the values must be "true", "10" and "foo".
This method translates the typed values into strings of the form needed by TypeSafe. Notice the loop over the "configProps", conversion to string, and the insertion into a newly created "effectiveProps."
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r108095955
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/QueryBuilder.java ---
@@ -271,6 +276,91 @@ public QuerySummary run() throws Exception {
}
/**
+ * Run the query and return the first result set as a
+ * {@link DirectRowSet} object that can be inspected directly
+ * by the code using a {@link RowSetReader}.
+ * <p>
+ * An enhancement is to provide a way to read a series of result
+ * batches as row sets.
+ * @return a row set that represents the first batch returned from
+ * the query
+ * @throws RpcException if anything goes wrong
+ */
+
+ public DirectRowSet rowSet() throws RpcException {
+
+ // Ignore all but the first non-empty batch.
+
+ QueryDataBatch dataBatch = null;
+ for (QueryDataBatch batch : results()) {
+ if (dataBatch == null && batch.getHeader().getRowCount() != 0) {
+ dataBatch = batch;
--- End diff --
break ? after first non-empty batch is found ?
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/788
Please see the initial comment which identifies that this PR depends on several others. Two have been done, only DRILL-5323 remains. Please commit DRILL-5323 first, then this one.
Note that, per your request, this PR is rebased onto DRILL-5323. The history here includes all changes from DRILL-5323 plus additional changes specific to DRILL-5318. As I understand it, if you first commit DRILL-5323 the common changes will reside in master, and will be ignored when you then (as a second step) commit this PR.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/788
It's fine to have example in a follow-up PR.
I was trying to see if I can use new operator/sub-operator test framework for the schema change task I'm doing. That requires to work with one or multiple RecordBatch (both data and operator).
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/788
Thanks for the changes. LGTM. +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 #788: DRILL-5318: Sub-operator test fixture
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/788
+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 pull request #788: DRILL-5318: Sub-operator test fixture
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r108563545
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/QueryBuilder.java ---
@@ -271,6 +276,91 @@ public QuerySummary run() throws Exception {
}
/**
+ * Run the query and return the first result set as a
+ * {@link DirectRowSet} object that can be inspected directly
+ * by the code using a {@link RowSetReader}.
+ * <p>
+ * An enhancement is to provide a way to read a series of result
+ * batches as row sets.
+ * @return a row set that represents the first batch returned from
+ * the query
+ * @throws RpcException if anything goes wrong
+ */
+
+ public DirectRowSet rowSet() throws RpcException {
+
+ // Ignore all but the first non-empty batch.
+
+ QueryDataBatch dataBatch = null;
+ for (QueryDataBatch batch : results()) {
+ if (dataBatch == null && batch.getHeader().getRowCount() != 0) {
+ dataBatch = batch;
+ } else {
+ batch.release();
+ }
+ }
+
+ // No results?
+
+ if (dataBatch == null) {
+ return null;
+ }
+
+ // Unload the batch and convert to a row set.
+
+ final RecordBatchLoader loader = new RecordBatchLoader(client.allocator());
+ try {
+ loader.load(dataBatch.getHeader().getDef(), dataBatch.getData());
+ dataBatch.release();
+ VectorContainer container = loader.getContainer();
+ container.setRecordCount(loader.getRecordCount());
+ return new DirectRowSet(client.allocator(), container);
+ } catch (SchemaChangeException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ /**
+ * Run the query that is expected to return (at least) one row
+ * with the only (or first) column returning a long value.
+ *
+ * @return the value of the first column of the first row
+ * @throws RpcException if anything goes wrong
+ */
+
+ public long singletonLong() throws RpcException {
+ RowSet rowSet = rowSet();
+ if (rowSet == null) {
+ throw new IllegalStateException("No rows returned");
+ }
+ RowSetReader reader = rowSet.reader();
+ reader.next();
+ long value = reader.column(0).getLong();
--- End diff --
Can we please refactor the common portion in singletonLong and singletonInt ? Since I guess in future if we support retrieving more types then again it will be repeated?
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/788
Rebased onto DRILL-5318 so that this branch includes the DRILL-5318 code that this PR depends upon, as well as the additional code required for this PR. Fixed the merge errors that caused the compilation issue.
Please commit DRILL-5318 first. Then, commit this PR. Given that this PR is already rebased on 5318, this PR should merge cleanly into master.
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r109555392
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/QueryBuilder.java ---
@@ -271,6 +276,91 @@ public QuerySummary run() throws Exception {
}
/**
+ * Run the query and return the first result set as a
+ * {@link DirectRowSet} object that can be inspected directly
+ * by the code using a {@link RowSetReader}.
+ * <p>
+ * An enhancement is to provide a way to read a series of result
+ * batches as row sets.
+ * @return a row set that represents the first batch returned from
+ * the query
+ * @throws RpcException if anything goes wrong
+ */
+
+ public DirectRowSet rowSet() throws RpcException {
+
+ // Ignore all but the first non-empty batch.
+
+ QueryDataBatch dataBatch = null;
+ for (QueryDataBatch batch : results()) {
+ if (dataBatch == null && batch.getHeader().getRowCount() != 0) {
+ dataBatch = batch;
--- End diff --
Ok makes sense after reading this: _"The reason for the funny code is that we want to release memory for all but the first batch."_
---
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 #788: DRILL-5318: Sub-operator test fixture
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/788#discussion_r111871234
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/ConfigBuilder.java ---
@@ -0,0 +1,130 @@
+/*
+ * 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.test;
+
+import java.util.Properties;
+import java.util.Map.Entry;
+
+import org.apache.drill.common.config.DrillConfig;
+
+/**
+ * Builds a {@link DrillConfig} for use in tests. Use this when a config
+ * is needed by itself, separate from an embedded Drillbit.
+ */
+public class ConfigBuilder {
+
+ protected String configResource;
+ protected Properties configProps;
+
+ /**
+ * Use the given configuration properties as overrides.
+ * @param configProps a collection of config properties
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder configProps(Properties configProps) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (this.configProps == null) {
+ this.configProps = configProps;
+ } else {
+ this.configProps.putAll(configProps);
+ }
+ return this;
+ }
+
+ /**
+ * Use the given configuration file, stored as a resource, to initialize
+ * the Drill config. Note that the resource file should have the two
+ * following settings to work as a config for an embedded Drillbit:
+ * <pre><code>
+ * drill.exec.sys.store.provider.local.write : false,
+ * drill.exec.http.enabled : false
+ * </code></pre>
+ * It may be more convenient to add your settings to the default
+ * config settings with {@link #configProperty(String, Object)}.
+ * @param configResource path to the file that contains the
+ * config file to be read
+ * @return this builder
+ * @see {@link #configProperty(String, Object)}
+ */
+
+ public ConfigBuilder resource(String configResource) {
+
+ if (configProps != null) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+
+ // TypeSafe gets unhappy about a leading slash, but other functions
+ // require it. Silently discard the leading slash if given to
+ // preserve the test writer's sanity.
+
+ this.configResource = ClusterFixture.trimSlash(configResource);
+ return this;
+ }
+
+ /**
+ * Add an additional boot-time property for the embedded Drillbit.
+ * @param key config property name
+ * @param value property value
+ * @return this builder
+ */
+
+ public ConfigBuilder put(String key, Object value) {
+ if (hasResource()) {
+ // Drill provides no constructor for this use case.
+ throw new IllegalArgumentException( "Cannot provide both a config resource and config properties.");
+ }
+ if (configProps == null) {
+ configProps = new Properties();
+ }
+ configProps.put(key, value.toString());
+ return this;
+ }
+
+ public DrillConfig build() {
+
+ // Create a config
+ // Because of the way DrillConfig works, we can set the ZK
+ // connection string only if a property set is provided.
+
+ if (hasResource()) {
+ return DrillConfig.create(configResource);
+ } else if (configProps != null) {
+ return DrillConfig.create(configProperties());
+ } else {
+ return DrillConfig.create();
+ }
+ }
+
+ private Properties configProperties() {
--- End diff --
Note that this has all been replaced by your recent addition...
---
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.
---