You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by paul-rogers <gi...@git.apache.org> on 2018/04/17 01:39:24 UTC
[GitHub] drill pull request #1213: Minor code cleanup
GitHub user paul-rogers opened a pull request:
https://github.com/apache/drill/pull/1213
Minor code cleanup
Pulled the remaining code cleanup items out of the Result Set work into this simple PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paul-rogers/drill DRILL-6334
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1213.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1213
----
commit 3439dfe8e20a8883c32a524ca0823c4bdfc10b53
Author: Paul Rogers <pr...@...>
Date: 2018-04-17T01:24:07Z
Minor code cleanup
----
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182440952
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestPromotableWriter.java ---
@@ -1,4 +1,4 @@
-/**
+/*
--- End diff --
All license headers should be already fixed as part of @ilooner PR #1207 or #1215.
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182445595
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/util/CallBack.java ---
@@ -1,4 +1,4 @@
-/**
+/*
--- End diff --
The same as the other license header
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182445689
--- Diff: exec/vector/src/test/java/org/apache/drill/exec/vector/VariableLengthVectorTest.java ---
@@ -1,4 +1,4 @@
-/**
+/*
--- End diff --
The same as the other license header
---
[GitHub] drill pull request #1213: DRILL-6334: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182590683
--- Diff: exec/vector/src/test/java/org/apache/drill/exec/vector/VariableLengthVectorTest.java ---
@@ -37,6 +37,7 @@ public void testSettingSameValueCount()
{
try (RootAllocator allocator = new RootAllocator(10_000_000)) {
final MaterializedField field = MaterializedField.create("stringCol", Types.required(TypeProtos.MinorType.VARCHAR));
+ @SuppressWarnings("resource")
--- End diff --
@paul-rogers Thank you for making the changes. Can you please double check whether it is necessary to suppress the warning here. It is confusing why the same warning suppression is removed in some places and added in others.
---
[GitHub] drill pull request #1213: DRILL-6334: Minor code cleanup
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182949848
--- Diff: exec/vector/src/test/java/org/apache/drill/exec/vector/VariableLengthVectorTest.java ---
@@ -37,6 +37,7 @@ public void testSettingSameValueCount()
{
try (RootAllocator allocator = new RootAllocator(10_000_000)) {
final MaterializedField field = MaterializedField.create("stringCol", Types.required(TypeProtos.MinorType.VARCHAR));
+ @SuppressWarnings("resource")
--- End diff --
These warnings may be unique to Eclipse. In this case, the vector is created, but not used in try-with-catch block which is the key purpose of anything that derives from `AutoCloseable`. Drill uses that class simply as a "close() without throwing exceptions" interface.
Since the warning really does not help in Drill, (and may not appear in IntelliJ), I can just turn the warning off in Eclipse so I'm not tempted to silence the warnings by inserting these annotations.
---
[GitHub] drill issue #1213: DRILL-6334: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1213
LGTM
---
[GitHub] drill pull request #1213: DRILL-6334: Minor code cleanup
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1213
---
[GitHub] drill issue #1213: Minor code cleanup
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1213
Should we have Jira for this? Otherwise, how it can be tracked for weekly batch commits?
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182444596
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestPromotableWriter.java ---
@@ -33,6 +33,7 @@
public void list() throws Exception {
BufferAllocator allocator = RootAllocatorFactory.newRoot(DrillConfig.create());
TestOutputMutator output = new TestOutputMutator(allocator);
+ @SuppressWarnings("resource")
--- End diff --
Why is it necessary to suppress resource not being closed warning here and OK to remove warning suppression in other places?
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r181932078
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/exception/OversizedAllocationException.java ---
@@ -1,4 +1,4 @@
-/**
+/*
--- End diff --
Should be already handled by Tim's PR #1207.
---
[GitHub] drill pull request #1213: Minor code cleanup
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1213#discussion_r182445146
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/exception/OversizedAllocationException.java ---
@@ -27,6 +27,7 @@
* {@link RecordBatch#next() iteration}.</p>
*
*/
+@SuppressWarnings("serial")
--- End diff --
Will it be better to provide `serialVersionUUID` instead of warning suppression?
---