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/06/06 21:02:39 UTC
[GitHub] drill pull request #851: DRILL-5518: Test framework enhancements
GitHub user paul-rogers opened a pull request:
https://github.com/apache/drill/pull/851
DRILL-5518: Test framework enhancements
* Create a SubOperatorTest base class to do routine setup and shutdown.
* Additional methods to simplify creating complex schemas with field
widths.
* Define a test workspace with plugin-specific options (as for the CSV
storage plugin)
* When verifying row sets, add methods to verify and release just the
"actual" batch in addition to the existing method for verify and free
both the actual and expected batches.
* Allow reading of row set values as object for generic comparisons.
* "Column builder" within schema builder to simplify building a single
MatrializedField for tests.
* Misc. code cleanup.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paul-rogers/drill DRILL-5518
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/851.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 #851
----
commit e95ed08915fa04dfdc1977ce899ef34114c96ff1
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-05-16T22:55:41Z
DRILL-5518: Test framework enhancements
* Create a SubOperatorTest base class to do routine setup and shutdown.
* Additional methods to simplify creating complex schemas with field
widths.
* Define a test workspace with plugin-specific options (as for the CSV
storage plugin)
* When verifying row sets, add methods to verify and release just the
"actual" batch in addition to the existing method for verify and free
both the actual and expected batches.
* Allow reading of row set values as object for generic comparisons.
* "Column builder" within schema builder to simplify building a single
MatrializedField for tests.
* Misc. code cleanup.
----
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123139892
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -96,27 +137,57 @@ public SchemaBuilder withSVMode(SelectionVectorMode svMode) {
public SchemaBuilder() { }
+ public SchemaBuilder(BatchSchema baseSchema) {
+ for (MaterializedField field : baseSchema) {
+ columns.add(field);
--- End diff --
Fixed.
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123140560
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -309,13 +317,38 @@ private void testDoubleRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Double.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals(Double.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
Casting is required because `assertEquals` does not know what to do with an `Object` as its second argument. The cast to `double` forces a cast to `Double`, then unboxing.
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123138037
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/SubOperatorTest.java ---
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ ******************************************************************************/
--- End diff --
Checkstyle does not care. In fact, earlier headers used to use a line of stars across the top as well, but the first three characters, /**, caused the comment to look like Javadoc, so we've been deprecating that style. Still, fixed this one as well.
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123139571
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -53,6 +53,47 @@
public class SchemaBuilder {
/**
+ * Build a column schema (AKA "materialized field") based on name and a
+ * variety of schema options. Every column needs a name and (minor) type,
+ * some may need a mode other than required, may need a width, may
+ * need scale and precision, and so on.
+ */
+
+ // TODO: Add map methods
+
+ public static class ColumnBuilder {
+ private String name;
+ private MajorType.Builder typeBuilder;
--- End diff --
Fixed.
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122858155
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -289,13 +295,15 @@ private void testFloatRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Float.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals((double) Float.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
Same as below for second cast
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122854555
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/SubOperatorTest.java ---
@@ -0,0 +1,37 @@
+/*
+ * 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 org.junit.AfterClass;
+import org.junit.BeforeClass;
+
+public class SubOperatorTest extends DrillTest {
+
+ protected static OperatorFixture fixture;
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ fixture = OperatorFixture.standardFixture();
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
--- End diff --
How about just `setup` and `tearDown` ? Since annotations @BeforeClass and @AfterClass makes the other part clear ?
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122855065
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -96,27 +137,57 @@ public SchemaBuilder withSVMode(SelectionVectorMode svMode) {
public SchemaBuilder() { }
+ public SchemaBuilder(BatchSchema baseSchema) {
+ for (MaterializedField field : baseSchema) {
+ columns.add(field);
--- End diff --
just call `add` method of `SchemaBuilder` instead of `columns.add(field)`
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r123397036
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/accessor/impl/TupleReaderImpl.java ---
@@ -101,8 +88,61 @@ public String getAsString(int colIndex) {
return "\"" + colReader.getString() + "\"";
case DECIMAL:
return colReader.getDecimal().toPlainString();
+ case ARRAY:
+ return getArrayAsString(colReader.array());
default:
throw new IllegalArgumentException("Unsupported type " + colReader.valueType());
}
}
+
+ private String bytesToString(byte[] value) {
+ StringBuilder buf = new StringBuilder()
+ .append("[");
+ int len = Math.min(value.length, 20);
+ for (int i = 0; i < len; i++) {
+ if (i > 0) {
+ buf.append(", ");
+ }
+ buf.append((int) value[i]);
+ }
+ if (value.length > len) {
+ buf.append("...");
+ }
+ buf.append("]");
+ return buf.toString();
+ }
+
+ private String getArrayAsString(ArrayReader array) {
+ StringBuilder buf = new StringBuilder();
+ buf.append("[");
+ for (int i = 0; i < array.size(); i++) {
+ if (i > 0) {
+ buf.append( ", " );
+ }
+ switch (array.valueType()) {
--- End diff --
For `getObject()` implementation it's throwing specifically `UnsupportedOperationException` for Map and Array type whereas for default type it throws `IllegalArgumentException`. I thought the idea was Map and Array are valid ArgumentType but not supported for now, hence the comment.
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122857402
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/accessor/impl/TupleReaderImpl.java ---
@@ -101,8 +88,61 @@ public String getAsString(int colIndex) {
return "\"" + colReader.getString() + "\"";
case DECIMAL:
return colReader.getDecimal().toPlainString();
+ case ARRAY:
+ return getArrayAsString(colReader.array());
default:
throw new IllegalArgumentException("Unsupported type " + colReader.valueType());
}
}
+
+ private String bytesToString(byte[] value) {
+ StringBuilder buf = new StringBuilder()
+ .append("[");
+ int len = Math.min(value.length, 20);
+ for (int i = 0; i < len; i++) {
+ if (i > 0) {
+ buf.append(", ");
+ }
+ buf.append((int) value[i]);
+ }
+ if (value.length > len) {
+ buf.append("...");
+ }
+ buf.append("]");
+ return buf.toString();
+ }
+
+ private String getArrayAsString(ArrayReader array) {
+ StringBuilder buf = new StringBuilder();
+ buf.append("[");
+ for (int i = 0; i < array.size(); i++) {
+ if (i > 0) {
+ buf.append( ", " );
+ }
+ switch (array.valueType()) {
--- End diff --
for valueType `MAP` and `ARRAY` we should `throw UnsupportedOperationException()`
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/851
+1 LGTM
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123138411
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/SubOperatorTest.java ---
@@ -0,0 +1,37 @@
+/*
+ * 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 org.junit.AfterClass;
+import org.junit.BeforeClass;
+
+public class SubOperatorTest extends DrillTest {
+
+ protected static OperatorFixture fixture;
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ fixture = OperatorFixture.standardFixture();
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
--- End diff --
There is a reason... Junit also allows a \@Before and \@After tag that are per-test actions. Those are often called `setup()` and `teardown()`. The static per-class versions are often called with the names used here. Still, renamed them to be a bit clearer.
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122855768
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -96,27 +137,57 @@ public SchemaBuilder withSVMode(SelectionVectorMode svMode) {
public SchemaBuilder() { }
+ public SchemaBuilder(BatchSchema baseSchema) {
+ for (MaterializedField field : baseSchema) {
+ columns.add(field);
+ }
+ }
+
public SchemaBuilder add(String pathName, MajorType type) {
- MaterializedField col = MaterializedField.create(pathName, type);
+ return add(MaterializedField.create(pathName, type));
+ }
+
+ public SchemaBuilder add(MaterializedField col) {
columns.add(col);
return this;
}
+ public static MaterializedField columnSchema(String pathName, MinorType type, DataMode mode) {
+ return MaterializedField.create(pathName,
+ MajorType.newBuilder()
--- End diff --
Why not use `SchemaBuilder.ColumnBuilder` here as well ?
---
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 #851: DRILL-5518: Test framework enhancements
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:
https://github.com/apache/drill/pull/851
+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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123567643
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/accessor/impl/TupleReaderImpl.java ---
@@ -101,8 +88,61 @@ public String getAsString(int colIndex) {
return "\"" + colReader.getString() + "\"";
case DECIMAL:
return colReader.getDecimal().toPlainString();
+ case ARRAY:
+ return getArrayAsString(colReader.array());
default:
throw new IllegalArgumentException("Unsupported type " + colReader.valueType());
}
}
+
+ private String bytesToString(byte[] value) {
+ StringBuilder buf = new StringBuilder()
+ .append("[");
+ int len = Math.min(value.length, 20);
+ for (int i = 0; i < len; i++) {
+ if (i > 0) {
+ buf.append(", ");
+ }
+ buf.append((int) value[i]);
+ }
+ if (value.length > len) {
+ buf.append("...");
+ }
+ buf.append("]");
+ return buf.toString();
+ }
+
+ private String getArrayAsString(ArrayReader array) {
+ StringBuilder buf = new StringBuilder();
+ buf.append("[");
+ for (int i = 0; i < array.size(); i++) {
+ if (i > 0) {
+ buf.append( ", " );
+ }
+ switch (array.valueType()) {
--- End diff --
Fixed.
---
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 #851: DRILL-5518: Test framework enhancements
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/851
---
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 #851: DRILL-5518: Test framework enhancements
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/851
Resolved merge conflicts. Retested on pre-commit tests. Now passes.
This PR needs a committer to review so we I can then merge the PR 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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122858130
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -309,13 +317,38 @@ private void testDoubleRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Double.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals(Double.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
I don't think explicit casting is required here since `getObject-->getDouble` already returns `double` type
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123140827
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/accessor/impl/TupleReaderImpl.java ---
@@ -101,8 +88,61 @@ public String getAsString(int colIndex) {
return "\"" + colReader.getString() + "\"";
case DECIMAL:
return colReader.getDecimal().toPlainString();
+ case ARRAY:
+ return getArrayAsString(colReader.array());
default:
throw new IllegalArgumentException("Unsupported type " + colReader.valueType());
}
}
+
+ private String bytesToString(byte[] value) {
+ StringBuilder buf = new StringBuilder()
+ .append("[");
+ int len = Math.min(value.length, 20);
+ for (int i = 0; i < len; i++) {
+ if (i > 0) {
+ buf.append(", ");
+ }
+ buf.append((int) value[i]);
+ }
+ if (value.length > len) {
+ buf.append("...");
+ }
+ buf.append("]");
+ return buf.toString();
+ }
+
+ private String getArrayAsString(ArrayReader array) {
+ StringBuilder buf = new StringBuilder();
+ buf.append("[");
+ for (int i = 0; i < array.size(); i++) {
+ if (i > 0) {
+ buf.append( ", " );
+ }
+ switch (array.valueType()) {
--- End diff --
Handled via the `default` clause?
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123140434
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -289,13 +295,15 @@ private void testFloatRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Float.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals((double) Float.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
Fixed.
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122854874
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -53,6 +53,47 @@
public class SchemaBuilder {
/**
+ * Build a column schema (AKA "materialized field") based on name and a
+ * variety of schema options. Every column needs a name and (minor) type,
+ * some may need a mode other than required, may need a width, may
+ * need scale and precision, and so on.
+ */
+
+ // TODO: Add map methods
+
+ public static class ColumnBuilder {
+ private String name;
+ private MajorType.Builder typeBuilder;
--- End diff --
private `final`
---
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 #851: DRILL-5518: Test framework enhancements
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/851
Rebased on latest 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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123567359
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -309,13 +317,38 @@ private void testDoubleRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Double.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals(Double.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
The trick here is to understand the difference between static and dynamic typing. The run-time type of `getObject()` is `Double`, which can be unboxed to an int. But, Java is a statically-typed language. The compile-time type of `getObject()` is `Object`.
For the two-argument form of `assertEquals()`, that is fine: Java will box the `int` to get an `Integer`, pass both to `assertEquals()` which will call `equals()` on the objects and things work.
But, the three-argument form has the signature `assertEquals(double expected, double actual, double delta)`, and the compiler does not have sufficient information to convert the `Object` return value to a `double`. Here is the error:
```
The method assertEquals(double, double, double) in the type Assert is not applicable for the arguments (double, Object, double)
```
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r123397308
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/test/RowSetTest.java ---
@@ -309,13 +317,38 @@ private void testDoubleRW() {
assertEquals(0, reader.column(0).getDouble(), 0.000001);
assertTrue(reader.next());
assertEquals(Double.MAX_VALUE, reader.column(0).getDouble(), 0.000001);
+ assertEquals(Double.MAX_VALUE, (double) reader.column(0).getObject(), 0.000001);
--- End diff --
Yes but my understanding is that `RowSetReader.column(0)` will return `ColumnReader` and you implemented `getObject` in `AbstractColumnReader` which is handing the type there. So `getObject` will return correct type of value ?
---
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 #851: DRILL-5518: Test framework enhancements
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/851#discussion_r122854686
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/SubOperatorTest.java ---
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ ******************************************************************************/
--- End diff --
Replace with just */. Not sure if `checkstyle` plugin is fine with this style.
---
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 #851: DRILL-5518: Test framework enhancements
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/851#discussion_r123140150
--- Diff: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/SchemaBuilder.java ---
@@ -96,27 +137,57 @@ public SchemaBuilder withSVMode(SelectionVectorMode svMode) {
public SchemaBuilder() { }
+ public SchemaBuilder(BatchSchema baseSchema) {
+ for (MaterializedField field : baseSchema) {
+ columns.add(field);
+ }
+ }
+
public SchemaBuilder add(String pathName, MajorType type) {
- MaterializedField col = MaterializedField.create(pathName, type);
+ return add(MaterializedField.create(pathName, type));
+ }
+
+ public SchemaBuilder add(MaterializedField col) {
columns.add(col);
return this;
}
+ public static MaterializedField columnSchema(String pathName, MinorType type, DataMode mode) {
+ return MaterializedField.create(pathName,
+ MajorType.newBuilder()
--- End diff --
Just saving an unnecessary object creation. The schema builder is for cases where we set more than the "basic three" properties. This method handles the vast majority of the cases in which we use just the "basic three".
---
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.
---