[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909355#comment-16909355 ] ASF GitHub Bot commented on DRILL-7350: --- 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909298#comment-16909298 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909296#comment-16909296 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909299#comment-16909299 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909300#comment-16909300 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909297#comment-16909297 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909295#comment-16909295 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909247#comment-16909247 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909236#comment-16909236 ] ASF GitHub Bot commented on DRILL-7350: --- 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909206#comment-16909206 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909191#comment-16909191 ] ASF GitHub Bot commented on DRILL-7350: --- 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909174#comment-16909174 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909178#comment-16909178 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909175#comment-16909175 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 &nash; 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909173#comment-16909173 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909177#comment-16909177 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909172#comment-16909172 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909176#comment-16909176 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16909143#comment-16909143 ] ASF GitHub Bot commented on DRILL-7350: --- paul-rogers commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7351) WebUI is Vulnerable to CSRF
[ https://issues.apache.org/jira/browse/DRILL-7351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908992#comment-16908992 ] Anton Gozhiy commented on DRILL-7351: - [~perialdon], there is a common agreement about what a bug report should contain: - [optional] Initial conditions - Steps to reproduce - Expected results - Actual results - Logs, screenshots and any additional info that would help to track it down >From your message it is not clear, what the problem exactly is and what use >cases it can affect. > WebUI is Vulnerable to CSRF > --- > > Key: DRILL-7351 > URL: https://issues.apache.org/jira/browse/DRILL-7351 > Project: Apache Drill > Issue Type: Bug > Components: Web Server >Affects Versions: 1.16.0 >Reporter: Don Perial >Priority: Major > Attachments: drill-csrf.html > > > There is no way to protect the WebUI from CSRF and the fact that the value > for the access-control-allow-origin header is '*' appears to confound this > issue as well. > The attached file demonstrates the vulnerability. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[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)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Description: Source - https://checkstyle.sourceforge.io/checks.html 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] * [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] * [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] * [UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] and others was: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] * [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] * [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] * [UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] and others > 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 >Priority: Major > Fix For: 1.17.0 > > > Source - https://checkstyle.sourceforge.io/checks.html > 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] > * [InnerTypeLast > |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] > * > [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] > * > [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] > * > [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] > * [UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] > and others -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Description: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] * [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] * [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] *[UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] and other was: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] and other > 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 >Priority: Major > Fix For: 1.17.0 > > > 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] > * [InnerTypeLast > |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] > * > [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] > * > [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] > * > [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] > *[UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] > and other -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Description: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] * [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] * [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] * [UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] and others was: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] * [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] * [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] *[UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] and other > 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 >Priority: Major > Fix For: 1.17.0 > > > 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] > * [InnerTypeLast > |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] > * > [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] > * > [InvalidJavadocPosition|https://checkstyle.sourceforge.io/config_javadoc.html#InvalidJavadocPosition] > * > [ArrayTypeStyle|https://checkstyle.sourceforge.io/config_misc.html#ArrayTypeStyle] > * [UpperEll|https://checkstyle.sourceforge.io/config_misc.html#UpperEll] > and others -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Description: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] * [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] and other was: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] and other > 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 >Priority: Major > Fix For: 1.17.0 > > > 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] > * [InnerTypeLast > |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] > * > [MissingOverride|https://checkstyle.sourceforge.io/config_annotation.html#MissingOverride] > and other -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Description: 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] * [InnerTypeLast |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] and other was: 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 > 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 >Priority: Major > Fix For: 1.17.0 > > > 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] > * [InnerTypeLast > |https://checkstyle.sourceforge.io/config_design.html#InnerTypeLast] > and other -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908984#comment-16908984 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Updated] (DRILL-7352) Introduce new checkstyle rules to make code style more consistent
[ https://issues.apache.org/jira/browse/DRILL-7352?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Arina Ielchiieva updated DRILL-7352: Fix Version/s: 1.17.0 > 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 >Priority: Major > Fix For: 1.17.0 > > > 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)
[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)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908979#comment-16908979 ] ASF GitHub Bot commented on DRILL-7350: --- 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908974#comment-16908974 ] ASF GitHub Bot commented on DRILL-7350: --- arina-ielchiieva commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908972#comment-16908972 ] ASF GitHub Bot commented on DRILL-7350: --- arina-ielchiieva commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908967#comment-16908967 ] ASF GitHub Bot commented on DRILL-7350: --- arina-ielchiieva commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908958#comment-16908958 ] ASF GitHub Bot commented on DRILL-7350: --- arina-ielchiieva commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908956#comment-16908956 ] ASF GitHub Bot commented on DRILL-7350: --- arina-ielchiieva commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908953#comment-16908953 ] ASF GitHub Bot commented on DRILL-7350: --- 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908947#comment-16908947 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908926#comment-16908926 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 &nash; 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908919#comment-16908919 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 &nash; 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908924#comment-16908924 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908927#comment-16908927 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908922#comment-16908922 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908921#comment-16908921 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908920#comment-16908920 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908925#comment-16908925 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908923#comment-16908923 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908918#comment-16908918 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Commented] (DRILL-7350) Move RowSet related classes from test folder
[ https://issues.apache.org/jira/browse/DRILL-7350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16908917#comment-16908917 ] ASF GitHub Bot commented on DRILL-7350: --- vvysotskyi commented on 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 > Move RowSet related classes from test folder > > > Key: DRILL-7350 > URL: https://issues.apache.org/jira/browse/DRILL-7350 > Project: Apache Drill > Issue Type: Task >Reporter: Volodymyr Vysotskyi >Assignee: Volodymyr Vysotskyi >Priority: Major > Fix For: 1.17.0 > > > Move RowSet related classes from test folder to main to be able to use them > for Metastore. -- This message was sent by Atlassian JIRA (v7.6.14#76016)