[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
Not sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
Not sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314672905
##
File path:
exec/java-exec/src/test/java/org/apache/drill/test/QueryRowSetIterator.java
##
@@ -99,6 +101,13 @@ public DirectRowSet next() {
}
}
+ public void printAll() {
Review comment:
No sure we should allow printing into stout in the code. When code was for
tests, it was ok but since we moved it, I think we should consider passing the
stream instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314670539
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -41,13 +38,19 @@ public AbstractRowSet(VectorContainer container,
TupleMetadata schema) {
}
@Override
- public VectorAccessible vectorAccessible() { return container(); }
+ public VectorAccessible vectorAccessible() {
Review comment:
I think we should enforce such checkstyle. Taking into account that most of
IDE can compress such methods automatically plus many other projects follow the
same pattern. Personally, I was following such code style as well. @vvysotskyi
could you please create Jira?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314669714
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractSingleRowSet.java
##
@@ -15,37 +15,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.drill.test.rowSet;
+package org.apache.drill.exec.physical.rowSet;
import org.apache.drill.exec.record.RecordBatchSizer;
import org.apache.drill.exec.physical.rowSet.model.ReaderIndex;
import
org.apache.drill.exec.physical.rowSet.model.MetadataProvider.MetadataRetrieval;
import org.apache.drill.exec.physical.rowSet.model.single.BaseReaderBuilder;
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.record.metadata.TupleMetadata;
-import org.apache.drill.test.rowSet.RowSet.SingleRowSet;
+import org.apache.drill.exec.physical.rowSet.RowSet.SingleRowSet;
/**
* Base class for row sets backed by a single record batch.
*/
public abstract class AbstractSingleRowSet extends AbstractRowSet implements
SingleRowSet {
- public static class RowSetReaderBuilder extends BaseReaderBuilder {
-
-public RowSetReader buildReader(AbstractSingleRowSet rowSet, ReaderIndex
rowIndex) {
- TupleMetadata schema = rowSet.schema();
- return new RowSetReaderImpl(schema, rowIndex,
- buildContainerChildren(rowSet.container(),
- new MetadataRetrieval(schema)));
-}
- }
-
- public AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
+ protected AbstractSingleRowSet(AbstractSingleRowSet rowSet) {
Review comment:
Agree on moving nested classes to the bottom since it makes code mode
readable, first you see all code that belongs to the class and then nested
classes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314382345 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java ## @@ -0,0 +1,111 @@ +/* + * 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. Review comment: It would be nice to add an example of the output. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314382220
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+public class RowSetStringBuilder {
+ private RowSet rowSet;
+
+ public RowSetStringBuilder(RowSet rowSet) {
+this.rowSet = rowSet;
+ }
+
+ public static void appendTupleSchema(StringBuilder stringBuilder,
TupleMetadata schema) {
+for (int i = 0; i < schema.size(); i++) {
+ if (i > 0) {
+stringBuilder.append(", ");
+ }
+ ColumnMetadata colSchema = schema.metadata(i);
+ stringBuilder.append(colSchema.name());
+ if (colSchema.isMap()) {
+stringBuilder.append("{");
+appendTupleSchema(stringBuilder, colSchema.mapSchema());
+stringBuilder.append("}");
+ }
+}
+ }
+
+ @Override
+ public String toString() {
+StringBuilder result = new StringBuilder();
+appendTo(result);
+return result.toString();
+ }
+
+ public void appendTo(StringBuilder stringBuilder) {
Review comment:
Same here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314378880
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+public class RowSetStringBuilder {
+ private RowSet rowSet;
+
+ public RowSetStringBuilder(RowSet rowSet) {
+this.rowSet = rowSet;
+ }
+
+ public static void appendTupleSchema(StringBuilder stringBuilder,
TupleMetadata schema) {
+for (int i = 0; i < schema.size(); i++) {
+ if (i > 0) {
+stringBuilder.append(", ");
+ }
+ ColumnMetadata colSchema = schema.metadata(i);
+ stringBuilder.append(colSchema.name());
+ if (colSchema.isMap()) {
+stringBuilder.append("{");
+appendTupleSchema(stringBuilder, colSchema.mapSchema());
+stringBuilder.append("}");
+ }
+}
+ }
+
+ @Override
+ public String toString() {
+StringBuilder result = new StringBuilder();
+appendTo(result);
+return result.toString();
+ }
+
+ public void appendTo(StringBuilder stringBuilder) {
+SelectionVectorMode selectionMode = rowSet.indirectionType();
+RowSetReader reader = rowSet.reader();
+int colCount = reader.tupleSchema().size();
+appendSchema(stringBuilder, selectionMode, reader);
+while (reader.next()) {
+ appendHeader(stringBuilder, reader, selectionMode);
+ for (int i = 0; i < colCount; i++) {
+if (i > 0) {
+ stringBuilder.append(", ");
+}
+stringBuilder.append(reader.column(i).getAsString());
+ }
+ stringBuilder.append("\n");
+}
+ }
+
+ private void appendSchema(StringBuilder stringBuilder, SelectionVectorMode
selectionMode, RowSetReader reader) {
+stringBuilder.append("#");
+switch (selectionMode) {
+ case FOUR_BYTE:
+stringBuilder.append(" (batch #, row #)");
+break;
+ case TWO_BYTE:
+stringBuilder.append(" (row #)");
+break;
+ default:
+break;
+}
+stringBuilder.append(": ");
+TupleMetadata schema = reader.tupleSchema();
+appendTupleSchema(stringBuilder, schema);
+stringBuilder.append("\n");
+ }
+
+ private void appendHeader(StringBuilder stringBuilder, RowSetReader reader,
SelectionVectorMode selectionMode) {
+stringBuilder.append(reader.logicalIndex());
+switch (selectionMode) {
+ case FOUR_BYTE:
+stringBuilder.append(" (");
+stringBuilder.append(reader.hyperVectorIndex());
+stringBuilder.append(", ");
+stringBuilder.append(reader.offset());
+stringBuilder.append(")");
+break;
+ case TWO_BYTE:
+stringBuilder.append(" (");
+stringBuilder.append(reader.offset());
+stringBuilder.append(")");
+break;
+ default:
Review comment:
Do we need default here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314378996
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+public class RowSetStringBuilder {
+ private RowSet rowSet;
Review comment:
final?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314382035
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetStringBuilder.java
##
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+public class RowSetStringBuilder {
+ private RowSet rowSet;
+
+ public RowSetStringBuilder(RowSet rowSet) {
+this.rowSet = rowSet;
+ }
+
+ public static void appendTupleSchema(StringBuilder stringBuilder,
TupleMetadata schema) {
Review comment:
Why this is public and static?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314321216
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -15,7 +15,7 @@
* 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;
Review comment:
1. Remove `protected SchemaChangeCallBack callBack = new
SchemaChangeCallBack();`
2. Better error message: `public long size() {
throw new UnsupportedOperationException("getSize");
}`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314320356 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetReader.java ## @@ -15,7 +15,7 @@ * 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; Review comment: Please rename method: `void setPosn(int index);` This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314319626 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetReader.java ## @@ -46,8 +46,7 @@ * The index of the underlying row which may be indexed by an * SV2 or SV4. * - * @return + * @return index of the underlying row */ - int offset(); } Review comment: New line. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314324081
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSet.java
##
@@ -15,7 +15,7 @@
* 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;
Review comment:
1. Remove public: `public interface SingleRowSet extends RowSet {`
2. Please try to fix all java doc errors.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314324678 ## File path: exec/java-exec/src/test/java/org/apache/drill/test/rowSet/RowSetBuilder.java ## @@ -20,12 +20,15 @@ import java.util.Set; import org.apache.drill.exec.memory.BufferAllocator; +import org.apache.drill.exec.physical.rowSet.DirectRowSet; +import org.apache.drill.exec.physical.rowSet.RowSet; +import org.apache.drill.exec.physical.rowSet.RowSetWriter; import org.apache.drill.exec.record.BatchSchema; import org.apache.drill.exec.record.metadata.MetadataUtils; import org.apache.drill.exec.record.metadata.TupleMetadata; import org.apache.drill.exec.vector.accessor.convert.ColumnConversionFactory; import org.apache.drill.shaded.guava.com.google.common.collect.Sets; -import org.apache.drill.test.rowSet.RowSet.SingleRowSet; +import org.apache.drill.exec.physical.rowSet.RowSet.SingleRowSet; Review comment: 1. Capacity here can use constant we introduce for `10`. 2. Also can be considered to be moved. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314323618 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/IndirectRowSet.java ## @@ -15,7 +15,7 @@ * 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; Review comment: Replace usage of `Sets.newHashSet()` This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314319854 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetPrinter.java ## @@ -15,7 +15,7 @@ * 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; Review comment: Make sure Printer returns strings instead of sout Maybe rename it to other name since now it won't print. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314321490 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractSingleRowSet.java ## @@ -15,15 +15,15 @@ * 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; Review comment: Constructors can be protected. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314322007
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/DirectRowSet.java
##
@@ -33,9 +32,10 @@
import org.apache.drill.exec.record.VectorContainer;
import org.apache.drill.exec.record.selection.SelectionVector2;
import org.apache.drill.exec.vector.accessor.convert.ColumnConversionFactory;
-import org.apache.drill.test.rowSet.RowSet.ExtendableRowSet;
-import org.apache.drill.test.rowSet.RowSetWriterImpl.WriterIndexImpl;
+import org.apache.drill.exec.physical.rowSet.RowSet.ExtendableRowSet;
+import org.apache.drill.exec.physical.rowSet.RowSetWriterImpl.WriterIndexImpl;
+import java.util.HashSet;
Review comment:
`public RowSetWriter writer() {
return writer(10);
}`
Create a constant instead with Java-doc.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314323421
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/DirectRowSet.java
##
@@ -132,7 +131,7 @@ public RowSetReader reader() {
@Override
public SingleRowSet toIndirect() {
-return new IndirectRowSet(this, Sets.newHashSet());
+return new IndirectRowSet(this, new HashSet<>());
Review comment:
Should be immutable?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move
RowSet related classes from test folder
URL: https://github.com/apache/drill/pull/1843#discussion_r314321216
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/AbstractRowSet.java
##
@@ -15,7 +15,7 @@
* 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;
Review comment:
1. Remove `protected SchemaChangeCallBack callBack = new
SchemaChangeCallBack();`
2.Better error message: `public long size() {
throw new UnsupportedOperationException("getSize");
}`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder
arina-ielchiieva commented on a change in pull request #1843: DRILL-7350: Move RowSet related classes from test folder URL: https://github.com/apache/drill/pull/1843#discussion_r314319854 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/rowSet/RowSetPrinter.java ## @@ -15,7 +15,7 @@ * 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; Review comment: Make sure Printer returns strings instead of sout This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
