[GitHub] [drill] vvysotskyi commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#issuecomment-522131852 The latest change caused tests failures. It is appeared because with new BatchSchemaBuilder TupleMetadata was used more frequently and some code does not set zero scale and precision, but PrimitiveColumnMetadata does. I'll let you know when this issue is fixed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314824297
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -17,24 +17,26 @@
*/
package org.apache.drill.test;
+import java.io.OutputStreamWriter;
import java.util.Iterator;
import org.apache.drill.exec.exception.SchemaChangeException;
import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.drill.exec.physical.rowSet.RowSetStringBuilder;
import org.apache.drill.exec.proto.UserBitShared.QueryId;
import org.apache.drill.exec.proto.UserBitShared.QueryResult.QueryState;
import org.apache.drill.exec.proto.helper.QueryIdHelper;
import org.apache.drill.exec.record.RecordBatchLoader;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.rpc.user.QueryDataBatch;
import org.apache.drill.test.BufferingQueryEventListener.QueryEvent;
-import org.apache.drill.test.rowSet.DirectRowSet;
+import org.apache.drill.exec.physical.rowSet.DirectRowSet;
public class QueryRowSetIterator implements Iterator,
Iterable {
private final BufferingQueryEventListener listener;
private int recordCount = 0;
private int batchCount = 0;
- QueryId queryId = null;
+ private QueryId queryId = null;
Review comment:
Thanks, done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314829114
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
No need to cast, it was an example. In the method where the value is used
reference type is already `Object`, so we don't need to do a cast.
> Unless we wrap that single column in another array, the second form is the
same as:
> ```
> ... addRow(new Object[] {10, "foo"});
> ```
Actually not, it will be the same as
```
addRow((Object) new Object[] {10, "foo"});
```
because method argument in `addSingleCol` has `Object` type.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314823770
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/log/TestLogReader.java
##
@@ -781,9 +781,19 @@ public void testPluginSerialization() throws IOException {
@Test
public void testFirewallSchema() throws RpcException {
-String sql = "SELECT * FROM cp.`regex/firewall.ssdlog`";
+String sql = "SELECT * FROM cp.`regex/firewall.ssdlog` limit 0";
RowSet result = client.queryBuilder().sql(sql).rowSet();
-result.print();
+TupleMetadata expectedSchema = new SchemaBuilder()
+.add("eventDate", TypeProtos.MinorType.TIMESTAMP,
TypeProtos.DataMode.OPTIONAL)
Review comment:
Thanks, done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314823714
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/log/TestLogReader.java
##
@@ -781,9 +781,19 @@ public void testPluginSerialization() throws IOException {
@Test
public void testFirewallSchema() throws RpcException {
-String sql = "SELECT * FROM cp.`regex/firewall.ssdlog`";
+String sql = "SELECT * FROM cp.`regex/firewall.ssdlog` limit 0";
RowSet result = client.queryBuilder().sql(sql).rowSet();
-result.print();
+TupleMetadata expectedSchema = new SchemaBuilder()
Review comment:
I went further, removed usage of another `SchemaBuilder` and removed the
class. We already have `BatchSchemaBuilder`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314809061
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,112 @@
+/*
+ * 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.physical.rowSet;
+
+import org.apache.drill.exec.record.BatchSchema.SelectionVectorMode;
+import org.apache.drill.exec.record.metadata.ColumnMetadata;
+import org.apache.drill.exec.record.metadata.TupleMetadata;
+
+/**
+ * Helper class to obtain string representation of RowSet.
+ * Example of the output:
+ *
+ * #: year, month, day
+ * 0: 2017, 12, 17
+ * 1: 2017, 12, 18
+ * 2: 2017, 12, 19
+ *
+ */
+public class RowSetStringBuilder {
Review comment:
Thanks, made the changes you have proposed.
Regarding writing to logs, we have `LogWriter` which implements `Writer`,
and with this class, we will be able to write to logs also.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314805977
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -101,7 +103,7 @@ public DirectRowSet next() {
public void printAll() {
for (DirectRowSet rowSet : this) {
- rowSet.print();
+ new RowSetStringBuilder(rowSet).writeTo(new
OutputStreamWriter(System.out));
Review comment:
Thanks, replaced it with `RowSetFormatter.print(rowSet);`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314816402
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
Ah! But I don't want to do the cast; that is the purpose of this method. I
want:
```
...addRow(mapValue(10, "foo"), 20); // convenience
...addRow(new Object[] {10, "foo"}, 20); // low-level
...
...addSingleCol(mapValue(10, "foo"); // convenience
... addSingleCol(new Object[] {10, "foo"}); // low-level
```
Unless we wrap that single column in another array, the second form is the
same as:
```
... addRow(new Object[] {10, "foo"});
```
Which will be interpreted as a set of two columns, not as a single column
with a map-value.
I could be wrong, and I'd have to test actual code to be sure. In fact, I'll
do that test this weekend.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#issuecomment-522085882 @arina-ielchiieva, could be my misunderstanding, though I do recall making the change... Point is, both forms are perfectly valid Java; it is just a preference. To avoid misunderstandings we might want to add to our formatting rules. Again, just to be clear, I have no issue with whatever rules we decide on; but let's decide on a common set to avoid ping-pong format changes as each of us touches the code. As it turns out, Charles started a discussion on the dev list. Perhaps you can offer there the format rules you've found to work, such as `int[] foo;` instead of `int foo[];`, classes-at-bottom, no single-line method declarations, preferred import sort order and so on. Might also include preferences for Java over Guava methods for common operations. That way, we can all follow the best practices you've discovered. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
Re: [DISCUSS]: Changes to Formatting Rules
Hi Charles, Agree. I am reviewing a PR in which formatting was changed. Both the original and revised formatting are fine, and each is often seen in open source projects. But we really should settle on one style so we don't have each of us reformatting code to our favorite styles. Drill does have documented standards, [1], but they are out of date with respect to the Checkstyle rules. The IntelliJ and Eclipse formatting templates are out of date with respect to the style preferred by some team members (and, it seems are inconsistent with the Checkstyle rules.) Not surprisingly, this inconsistency leads to a bit of chaos and uncertainty. Code formatting is a hassle and the "right" style is always what any one person prefers. Still, might be worth while discussing and formalizing our current preferences so we can update the format templates to match. Perhaps each of us can toss out ideas for either a) the preferred formatting not covered by Checkstyle, or b) where the format templates and Checkstyle rules conflict. Would we prefer to do that here, or via a JIRA ticket? Thanks, - Paul [1] http://drill.apache.org/docs/apache-drill-contribution-guidelines/ On Friday, August 16, 2019, 09:21:51 AM PDT, Charles Givre wrote: Hello all, I recently submitted a PR and after running the code formatter in IntelliJ, there still were a lot of formatting issues. Personally, I don't really care what the conventions are that we uset, but is it possible to have the formatting template in IntelliJ match the Drill requirements? -- C
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314798843
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
Tests passed with this change. I'm afraid that we are talking about
different cases.
`addRow(new Object[]{"foo", 10})` will work as you expected - it will handle
incoming array as two column values, but the code in the method which was
changed is different a little bit, it is similar to this call:
`addRow((Object) new Object[]{"foo", 10})` - in this case, it will be
handled as a single column with array value.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#issuecomment-522068340 @paul-rogers > I also recall that Arina once suggested removing an anonymous inner class in favor of a declared class. I doubt this is true :) Maybe thrown out of the context though... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[DISCUSS]: Changes to Formatting Rules
Hello all, I recently submitted a PR and after running the code formatter in IntelliJ, there still were a lot of formatting issues. Personally, I don't really care what the conventions are that we uset, but is it possible to have the formatting template in IntelliJ match the Drill requirements? -- C
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314788921
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/log/TestLogReader.java
##
@@ -781,9 +781,19 @@ public void testPluginSerialization() throws IOException {
@Test
public void testFirewallSchema() throws RpcException {
-String sql = "SELECT * FROM cp.`regex/firewall.ssdlog`";
+String sql = "SELECT * FROM cp.`regex/firewall.ssdlog` limit 0";
RowSet result = client.queryBuilder().sql(sql).rowSet();
-result.print();
+TupleMetadata expectedSchema = new SchemaBuilder()
Review comment:
Another change you might consider is renaming either this, or the other
`SchemaBuilder`. Since this is the one we want to use moving forward, maybe
rename the other to `BatchSchemaBuilder` so that future folks don't have to
figure out the correct import when the reference the class.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314787920
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -101,7 +103,7 @@ public DirectRowSet next() {
public void printAll() {
for (DirectRowSet rowSet : this) {
- rowSet.print();
+ new RowSetStringBuilder(rowSet).writeTo(new
OutputStreamWriter(System.out));
Review comment:
This is *not* a simplification; it exposes too much detail. Better:
```
RowSetFormatter.print(rowSet);
```
Of course, if we go this route, we might as well leave the `print()` method
in `RowSet` and have it call the above code.
Just to be clear, the purpose of `print()` is to help us developers
visualize what's going on in Drill; it is not for production use. This is
important because, if you can't visualize a batch, you'll make many mistakes
because you can't tell what's what.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314786038 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/package-info.java ## @@ -32,6 +32,22 @@ * batch" or "record batch." (But, in Drill, the term "record batch" also * usually means an operator on that set of records. Here, a row set is * just the rows separate from operations on that data. + * SingleRowSet (abstract) Review comment: Thanks! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314783903
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractSingleRowSet.java
##
@@ -15,37 +15,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.drill.test.rowSet;
+package org.apache.drill.exec.physical.rowSet;
import org.apache.drill.exec.record.RecordBatchSizer;
import org.apache.drill.exec.physical.rowSet.model.ReaderIndex;
import
org.apache.drill.exec.physical.rowSet.model.MetadataProvider.MetadataRetrieval;
import org.apache.drill.exec.physical.rowSet.model.single.BaseReaderBuilder;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.record.metadata.TupleMetadata;
-import org.apache.drill.test.rowSet.RowSet.SingleRowSet;
+import org.apache.drill.exec.physical.rowSet.RowSet.SingleRowSet;
/**
* Base class for row sets backed by a single record batch.
*/
public abstract class AbstractSingleRowSet extends AbstractRowSet implements
SingleRowSet {
- public static class RowSetReaderBuilder extends BaseReaderBuilder {
-
-public RowSetReader buildReader(AbstractSingleRowSet rowSet, ReaderIndex
rowIndex) {
- TupleMetadata schema = rowSet.schema();
- return new RowSetReaderImpl(schema, rowIndex,
- buildContainerChildren(rowSet.container(),
- new MetadataRetrieval(schema)));
-}
- }
-
- public AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
+ protected AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
Review comment:
I can live with this, though the nested-classes at top is a more common
pattern in the Java community. The general rule is that constants and nested
classes at top, then fields, then methods. Original Java formatters tended to
put fields at the bottom. Some projects mix nested classes with methods: the
class is defined adjacent to the method that uses it.
I also recall that Arina once suggested removing an anonymous inner class in
favor of a declared class. This is, again, a style issue since anonymous inner
classes are a very common style, especially in UI and functional programming.
In general, we need to establish rules for the project, else each person
that touches the file will reformat code to their preferred style. We should
discuss those rules on the dev list before we start enforcing them.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314789387
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -17,24 +17,26 @@
*/
package org.apache.drill.test;
+import java.io.OutputStreamWriter;
import java.util.Iterator;
import org.apache.drill.exec.exception.SchemaChangeException;
import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.drill.exec.physical.rowSet.RowSetStringBuilder;
import org.apache.drill.exec.proto.UserBitShared.QueryId;
import org.apache.drill.exec.proto.UserBitShared.QueryResult.QueryState;
import org.apache.drill.exec.proto.helper.QueryIdHelper;
import org.apache.drill.exec.record.RecordBatchLoader;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.rpc.user.QueryDataBatch;
import org.apache.drill.test.BufferingQueryEventListener.QueryEvent;
-import org.apache.drill.test.rowSet.DirectRowSet;
+import org.apache.drill.exec.physical.rowSet.DirectRowSet;
public class QueryRowSetIterator implements Iterator,
Iterable {
private final BufferingQueryEventListener listener;
private int recordCount = 0;
private int batchCount = 0;
- QueryId queryId = null;
+ private QueryId queryId = null;
Review comment:
As long as you are touching this code, please remove the do-nothing
initializers:
```
private int recordCount;
private int batchCount;
private QueryId queryId;
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314785834
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,112 @@
+/*
+ * 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.physical.rowSet;
+
+import org.apache.drill.exec.record.BatchSchema.SelectionVectorMode;
+import org.apache.drill.exec.record.metadata.ColumnMetadata;
+import org.apache.drill.exec.record.metadata.TupleMetadata;
+
+/**
+ * Helper class to obtain string representation of RowSet.
+ * Example of the output:
+ *
+ * #: year, month, day
+ * 0: 2017, 12, 17
+ * 1: 2017, 12, 18
+ * 2: 2017, 12, 19
+ *
+ */
+public class RowSetStringBuilder {
Review comment:
The class name is still "RowSetStringBuilder" Better to rename it to, say,
"RowSetFormatter". The constructor should take both a row set and a writer. We
can then provide static methods for the two most common cases: print to stdout
(which I use all the time when creating tests) and to string (which you seem to
prefer):
```
public static void print(RowSet rowSet) {
new RowSetFormatter(rowSet, System.out).write();
}
public static String toString(RowSet rowSet) {
StringWriter out = new StringWriter();
new RowSetFormatter(rowSet, out).write();
return out.toString();
}
```
The constructor allows other options, such as writing to a file. Sadly, it
won't work to write to a log; we'd have to materialize to a string...
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314787032
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/log/TestLogReader.java
##
@@ -781,9 +781,19 @@ public void testPluginSerialization() throws IOException {
@Test
public void testFirewallSchema() throws RpcException {
-String sql = "SELECT * FROM cp.`regex/firewall.ssdlog`";
+String sql = "SELECT * FROM cp.`regex/firewall.ssdlog` limit 0";
RowSet result = client.queryBuilder().sql(sql).rowSet();
-result.print();
+TupleMetadata expectedSchema = new SchemaBuilder()
+.add("eventDate", TypeProtos.MinorType.TIMESTAMP,
TypeProtos.DataMode.OPTIONAL)
Review comment:
Simpler: `addNullable("eventDate", MinorType.TIMESTAMP)`
Import `MinorType`, use the `addNullable` method rather than the more
verbose form of using the `DataMode`. That is, express intent, not
implementation.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
paul-rogers commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314780985
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
While this would seem to be true, it won't work for the case of a map:
addRow(new Object[]{"foo", 10})
Is the above two column values, or a single column that happens to be an
object?
There is actually no need to debate this, however. Without this feature,
some unit tests will fail. (I know that because I added this method directly in
response to those test failures...)
Still, if you can get those tests to pass, even with the map value
ambiguity, then my concern will be addressed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[jira] [Created] (DRILL-7353) Found a bug in the JDBC driver drill-jdbc-all-1.16.0.jar
Anastasiia Sergienko created DRILL-7353:
---
Summary: Found a bug in the JDBC driver drill-jdbc-all-1.16.0.jar
Key: DRILL-7353
URL: https://issues.apache.org/jira/browse/DRILL-7353
Project: Apache Drill
Issue Type: Bug
Affects Versions: 1.16.0, 1.15.0
Reporter: Anastasiia Sergienko
Attachments: Screenshot from 2019-08-16 13-26-29.png
Hello. I used a JDBC driver for connection to the Apache Drill and probably
found a bug.
My problem was the driver didn't register itself automatically and I needed to
use `Class.forName("org.apache.drill.jdbc.Driver");` in Java.
I opened the jar archive(drill-jdbc-all-1.16.0.jar) and checked the next file:
/META-INF/services/java.sql.Driver.
What I found there was: org.apache.calcite.avatica.remote.Driver, which doesn't
correspond to the real driver path. I changed the line to:
org.apache.drill.jdbc.Driver
After that my problem was solved. I also checked the 1.15.0 version and the
path there was the same. Is it supposed to be like that?
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314679601
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -41,13 +38,19 @@ public AbstractRowSet(VectorContainer container,
TupleMetadata schema) {
}
@Override
- public VectorAccessible vectorAccessible() { return container(); }
+ public VectorAccessible vectorAccessible() {
Review comment:
I have created https://issues.apache.org/jira/browse/DRILL-7352 for this.
I'll populate it with more rules soon.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[jira] [Created] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
Volodymyr Vysotskyi created DRILL-7352: -- Summary: Introduce new checkstyle rules to make code style more consistent Key: DRILL-7352 URL: https://issues.apache.org/jira/browse/DRILL-7352 Project: Apache Drill Issue Type: Task Reporter: Volodymyr Vysotskyi List of rules to be enabled: * [LeftCurly|https://checkstyle.sourceforge.io/config_blocks.html#LeftCurly] - force placement of a left curly brace at the end of the line. * [RightCurly|https://checkstyle.sourceforge.io/config_blocks.html#RightCurly] - force placement of a right curly brace * [NewlineAtEndOfFile|https://checkstyle.sourceforge.io/config_misc.html#NewlineAtEndOfFile] * [UnnecessaryParentheses|https://checkstyle.sourceforge.io/config_coding.html#UnnecessaryParentheses] * [MethodParamPad|https://checkstyle.sourceforge.io/config_whitespace.html#MethodParamPad] and other -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[GitHub] [drill] arina-ielchiieva commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#issuecomment-521974104 Overall, LGTM. Let's wait for @paul-rogers approval. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
Not sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
Not sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
No sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314670539
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -41,13 +38,19 @@ public AbstractRowSet(VectorContainer container,
TupleMetadata schema) {
}
@Override
- public VectorAccessible vectorAccessible() { return container(); }
+ public VectorAccessible vectorAccessible() {
Review comment:
I think we should enforce such checkstyle. Taking into account that most of
IDE can compress such methods automatically plus many other projects follow the
same pattern. Personally, I was following such code style as well. @vvysotskyi
could you please create Jira?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314669714
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractSingleRowSet.java
##
@@ -15,37 +15,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.drill.test.rowSet;
+package org.apache.drill.exec.physical.rowSet;
import org.apache.drill.exec.record.RecordBatchSizer;
import org.apache.drill.exec.physical.rowSet.model.ReaderIndex;
import
org.apache.drill.exec.physical.rowSet.model.MetadataProvider.MetadataRetrieval;
import org.apache.drill.exec.physical.rowSet.model.single.BaseReaderBuilder;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.record.metadata.TupleMetadata;
-import org.apache.drill.test.rowSet.RowSet.SingleRowSet;
+import org.apache.drill.exec.physical.rowSet.RowSet.SingleRowSet;
/**
* Base class for row sets backed by a single record batch.
*/
public abstract class AbstractSingleRowSet extends AbstractRowSet implements
SingleRowSet {
- public static class RowSetReaderBuilder extends BaseReaderBuilder {
-
-public RowSetReader buildReader(AbstractSingleRowSet rowSet, ReaderIndex
rowIndex) {
- TupleMetadata schema = rowSet.schema();
- return new RowSetReaderImpl(schema, rowIndex,
- buildContainerChildren(rowSet.container(),
- new MetadataRetrieval(schema)));
-}
- }
-
- public AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
+ protected AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
Review comment:
Agree on moving nested classes to the bottom since it makes code mode
readable, first you see all code that belongs to the class and then nested
classes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on issue #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#issuecomment-521967080 @paul-rogers, I have reverted debatable formatting changes. Let's postpone this discussion until we decide to add some additional checkstyle rules. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314448553
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
I made this change because IDE proposed it but after some investigation, I
still think that it will work as it is expected. Java determines whether to
treat the incoming argument as a single element or as an array for vararg
functions at the compilation stage. So for the following case code will work as
you have described - the vararg will be a one-dimensional array of strings:
```
addRow(new String[]{"a", "b"});
```
But the following case will have different behavior - vararg will be treated
as a two-dimensional array:
```
Object s = new String[]{"a", "a"};
addRow(s);
```
So code here will work as it is expected.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314640505
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,112 @@
+/*
+ * 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.physical.rowSet;
+
+import org.apache.drill.exec.record.BatchSchema.SelectionVectorMode;
+import org.apache.drill.exec.record.metadata.ColumnMetadata;
+import org.apache.drill.exec.record.metadata.TupleMetadata;
+
+/**
+ * Helper class to obtain string representation of RowSet.
+ * Example of the output:
+ *
+ * #: year, month, day
+ * 0: 2017, 12, 17
+ * 1: 2017, 12, 18
+ * 2: 2017, 12, 19
+ *
+ */
+public class RowSetStringBuilder {
Review comment:
Good point, thanks, reworked this class to use `Writer`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314656744
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,13 +99,6 @@ public DirectRowSet next() {
}
}
- public void printAll() {
Review comment:
Reverted removing this method.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314655457
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/package-info.java
##
@@ -117,12 +134,14 @@
* are added while loading, their index is always at the end of the existing
* columns.
* Writing Data to the Batch
- * Each batch is delimited by a call to {@link #startBatch()} and a call to
- * {@link #harvestWithLookAhead()} to obtain the completed batch. Note that
readers do not
+ * Each batch is delimited by a call to {@link
org.apache.drill.exec.physical.rowSet.ResultSetLoader#startBatch()}
+ * and a call to {@link
org.apache.drill.exec.physical.rowSet.impl.VectorState#harvestWithLookAhead()}
+ * to obtain the completed batch. Note that readers do not
* call these methods; the scan operator does this work.
*
- * Each row is delimited by a call to {@link #startValue()} and a call to
- * {@link #saveRow()}. startRow() performs initialization necessary
+ * Each row is delimited by a call to {@link
org.apache.drill.exec.vector.accessor.writer.WriterEvents#startRow()}
Review comment:
Thanks for pointing this, reverted this change.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314436298
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/DirectRowSet.java
##
@@ -141,5 +136,22 @@ public SingleRowSet toIndirect(Set skipIndices) {
}
@Override
- public SelectionVector2 getSv2() { return null; }
+ public SelectionVector2 getSv2() {
+return null;
+ }
+
+ public static class RowSetWriterBuilder extends BaseWriterBuilder {
Review comment:
These classes are still nested and placed in the same top-level class, but
they have moved to the bottom of the top-level class.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314432299
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -41,13 +38,19 @@ public AbstractRowSet(VectorContainer container,
TupleMetadata schema) {
}
@Override
- public VectorAccessible vectorAccessible() { return container(); }
+ public VectorAccessible vectorAccessible() {
Review comment:
One-line methods are prevented in a lot of projects. Here is a couple of the
checkstyle rules which forbid such style:
https://checkstyle.sourceforge.io/config_blocks.html#LeftCurly and
https://checkstyle.sourceforge.io/config_blocks.html#RightCurly. I think at
some point, we will update the code to enable these and other rules.
Also, most of IDE can compress displaying such methods automatically.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314434313 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java ## @@ -15,23 +15,20 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.drill.test.rowSet; +package org.apache.drill.exec.physical.rowSet; import org.apache.drill.exec.memory.BufferAllocator; import org.apache.drill.exec.record.BatchSchema; import org.apache.drill.exec.record.VectorAccessible; import org.apache.drill.exec.record.VectorContainer; import org.apache.drill.exec.record.metadata.TupleMetadata; -import org.apache.drill.exec.vector.SchemaChangeCallBack; /** * Basic implementation of a row set for both the single and multiple - * (hyper) varieties, both the fixed and extendible varieties. + * (hyper) varieties, both the fixed and extendable varieties. Review comment: Thanks, done. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314648059
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
Actually, it should work as expected. Java determines how to treat incoming
arguments at the compilation stage, so if the reference has a non-array type,
it will be treated as a single element of vararg even if actual type of the
object is an array.
Here is these two different cases:
```
addRow(new String[]{"a", "a"});
```
and the same case as in the code:
```
Object value = new String[]{"a", "a"};
addRow(value);
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314435143
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractSingleRowSet.java
##
@@ -15,37 +15,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.drill.test.rowSet;
+package org.apache.drill.exec.physical.rowSet;
import org.apache.drill.exec.record.RecordBatchSizer;
import org.apache.drill.exec.physical.rowSet.model.ReaderIndex;
import
org.apache.drill.exec.physical.rowSet.model.MetadataProvider.MetadataRetrieval;
import org.apache.drill.exec.physical.rowSet.model.single.BaseReaderBuilder;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.record.metadata.TupleMetadata;
-import org.apache.drill.test.rowSet.RowSet.SingleRowSet;
+import org.apache.drill.exec.physical.rowSet.RowSet.SingleRowSet;
/**
* Base class for row sets backed by a single record batch.
*/
public abstract class AbstractSingleRowSet extends AbstractRowSet implements
SingleRowSet {
- public static class RowSetReaderBuilder extends BaseReaderBuilder {
-
-public RowSetReader buildReader(AbstractSingleRowSet rowSet, ReaderIndex
rowIndex) {
- TupleMetadata schema = rowSet.schema();
- return new RowSetReaderImpl(schema, rowIndex,
- buildContainerChildren(rowSet.container(),
- new MetadataRetrieval(schema)));
-}
- }
-
- public AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
+ protected AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
Review comment:
These classes weren't removed, but they moved to the bottom of the top-level
class. I believe we follow such a style in the project.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet
related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314448553
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetWriterImpl.java
##
@@ -129,7 +129,7 @@ public RowSetWriter addRow(Object...values) {
@Override
public RowSetWriter addSingleCol(Object value) {
-return addRow(new Object[] {value});
+return addRow(value);
Review comment:
I made this change because IDE proposed it but after some investigation, I
still think that it will work as it is expected. Java determines whether to
treat the incoming argument as a single element or as an array for vararg
functions at the compilation stage. So for the following case code will work as
you have described - the vararg will be a one-dimensional array of strings:
```
addRow(new String[]{"a", "b"});
```
But the following case will have different behavior - vararg will be treated
as a two-dimensional array:
```
Object s = new String[]{"a", "a"};
addRow(s);
```
So code here will work as it is expected.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314649381 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/package-info.java ## @@ -32,6 +32,22 @@ * batch" or "record batch." (But, in Drill, the term "record batch" also * usually means an operator on that set of records. Here, a row set is * just the rows separate from operations on that data. + * SingleRowSet (abstract) Review comment: Moved `resultSet` classes into a separate package and updated `package-info.java`. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
vvysotskyi commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314652572 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/package-info.java ## @@ -32,6 +32,22 @@ * batch" or "record batch." (But, in Drill, the term "record batch" also * usually means an operator on that set of records. Here, a row set is * just the rows separate from operations on that data. + * SingleRowSet (abstract) + * Represents a row set that contains a single record batch (the typical + * case. + * DirectRowSet + * A read-only single row set without a selection vector. + * IndirectRowSet + * A read-only, single row set with an SV2. Note that the SV2 itself is + * writable (such as for sorting.) + * ExtendableRowSet + * A write-only, single row set used to create a new row set. Because of + * the way Drill sets row counts, an extendable row set cannot be read; instead + * at the completion of the write the extendable row set becomes a direct or + * indirect row set. + * HyperRowSet + * A read-only row set made up of a collection of record batches, indexed via an + * SV4. As with the SV2, the SV4 itself is writable. Review comment: This part was moved from `package-info.java` in tests package. Removed this sentence to avoid confusion. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
