[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1085


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r162512667
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java ---
@@ -278,22 +273,62 @@ public void addDoubleMetrics(OperatorProfile.Builder 
builder) {
 }
   }
 
-  @Override
+  /**
+   * Set a stat to the specified long value. Creates the stat
+   * if the stat does not yet exist.
+   *
+   * @param metric the metric to update
+   * @param value the value to set
+   */
+
   public void addLongStat(MetricDef metric, long value){
 longMetrics.putOrAdd(metric.metricId(), value, value);
   }
 
-  @Override
+  @VisibleForTesting
+  public long getLongStat(MetricDef metric) {
+return longMetrics.get(metric.metricId());
--- End diff --

Presumably fail. But, since this used in testing to ensure that a long 
metric was, in fact, written, failure is actually a useful result.


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r162512111
  
--- Diff: 
common/src/main/java/org/apache/drill/common/exceptions/UserException.java ---
@@ -83,23 +83,17 @@ public static Builder memoryError() {
* The cause message will be used unless {@link 
Builder#message(String, Object...)} is called.
* If the wrapped exception is, or wraps, a user exception it will be 
returned by {@link Builder#build(Logger)}
* instead of creating a new exception. Any added context will be added 
to the user exception as well.
-   * 
-   * This exception, previously deprecated, has been repurposed to 
indicate unspecified
-   * errors. In particular, the case in which a lower level bit of code 
throws an
-   * exception other than UserException. The catching code then only knows 
"something went
-   * wrong", but not enough information to categorize the error.
-   * 
-   * System errors also indicate illegal internal states, missing 
functionality, and other
-   * code-related errors -- all of which "should never occur."
*
* @see 
org.apache.drill.exec.proto.UserBitShared.DrillPBError.ErrorType#SYSTEM
*
* @param cause exception we want the user exception to wrap. If cause 
is, or wrap, a user exception it will be
*  returned by the builder instead of creating a new user 
exception
* @return user exception builder
*
+   * @deprecated This method should never need to be used explicitly, 
unless you are passing the exception to the
+   * Rpc layer or UserResultListener.submitFailed()
*/
-
+  @Deprecated
--- End diff --

Good catch. Merge error. It is deprecated in master; I have undeprecated it 
in my branch...


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r162513175
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java 
---
@@ -38,20 +38,26 @@
 import org.apache.drill.exec.server.options.SystemOptionManager;
 import 
org.apache.drill.exec.store.sys.store.provider.LocalPersistentStoreProvider;
 import org.apache.drill.exec.util.GuavaPatcher;
+import org.apache.drill.test.BaseDirTestWatcher;
--- End diff --

Checked the source. No hidden characters. For me, formatting does appear 
below this line.

I suspect your browser hit some limit. Large PRs make Safari struggle to 
display the code. Suggestion: refresh the page. search for this file, and keep 
going. Perhaps, if you don't expand prior files, your browser will handle the 
formatting. (GitHub should provide some solution for large PRs...)


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r162512386
  
--- Diff: common/src/main/java/org/apache/drill/common/types/Types.java ---
@@ -728,4 +733,49 @@ public static boolean isLaterType(MajorType type) {
 return type.getMinorType() == MinorType.LATE;
   }
 
+  public static boolean isEquivalent(MajorType type1, MajorType type2) {
+
+// Requires full type equality, including fields such as precision and 
scale.
+// But, unset fields are equivalent to 0. Can't use the 
protobuf-provided
+// isEquals() which treats set and unset fields as different.
+
+if (type1.getMinorType() != type2.getMinorType() ||
+type1.getMode() != type2.getMode() ||
+type1.getScale() != type2.getScale() ||
+type1.getPrecision() != type2.getPrecision()) {
+  return false;
+}
+
+// Subtypes are only for unions and are seldom used.
+
+if (type1.getMinorType() != MinorType.UNION) {
+  return true;
+}
+
+List subtypes1 = type1.getSubTypeList();
+List subtypes2 = type2.getSubTypeList();
+if (subtypes1 == subtypes2) { // Only occurs if both are null
+  return true;
+}
+if (subtypes1 == null || subtypes2 == null) {
+  return false;
+}
+if (subtypes1.size() != subtypes2.size()) {
+  return false;
+}
+
+// Now it gets slow because subtype lists are not ordered.
+
+List copy1 = new ArrayList<>();
+List copy2 = new ArrayList<>();
+copy1.addAll(subtypes1);
+copy2.addAll(subtypes2);
+Collections.sort(copy1);
+Collections.sort(copy2);
+return copy1.equals(copy2);
+  }
+
+  public static String typeKey(MinorType type) {
--- End diff --

Done.


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r161208584
  
--- Diff: common/src/main/java/org/apache/drill/common/types/Types.java ---
@@ -728,4 +733,49 @@ public static boolean isLaterType(MajorType type) {
 return type.getMinorType() == MinorType.LATE;
   }
 
+  public static boolean isEquivalent(MajorType type1, MajorType type2) {
+
+// Requires full type equality, including fields such as precision and 
scale.
+// But, unset fields are equivalent to 0. Can't use the 
protobuf-provided
+// isEquals() which treats set and unset fields as different.
+
+if (type1.getMinorType() != type2.getMinorType() ||
+type1.getMode() != type2.getMode() ||
+type1.getScale() != type2.getScale() ||
+type1.getPrecision() != type2.getPrecision()) {
+  return false;
+}
+
+// Subtypes are only for unions and are seldom used.
+
+if (type1.getMinorType() != MinorType.UNION) {
+  return true;
+}
+
+List subtypes1 = type1.getSubTypeList();
+List subtypes2 = type2.getSubTypeList();
+if (subtypes1 == subtypes2) { // Only occurs if both are null
+  return true;
+}
+if (subtypes1 == null || subtypes2 == null) {
+  return false;
+}
+if (subtypes1.size() != subtypes2.size()) {
+  return false;
+}
+
+// Now it gets slow because subtype lists are not ordered.
+
+List copy1 = new ArrayList<>();
+List copy2 = new ArrayList<>();
+copy1.addAll(subtypes1);
+copy2.addAll(subtypes2);
+Collections.sort(copy1);
+Collections.sort(copy2);
+return copy1.equals(copy2);
+  }
+
+  public static String typeKey(MinorType type) {
--- End diff --

Why we need this method? It looks like it is never used? If it's intended 
for future use please add java doc then.


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r161215163
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java 
---
@@ -38,20 +38,26 @@
 import org.apache.drill.exec.server.options.SystemOptionManager;
 import 
org.apache.drill.exec.store.sys.store.provider.LocalPersistentStoreProvider;
 import org.apache.drill.exec.util.GuavaPatcher;
+import org.apache.drill.test.BaseDirTestWatcher;
--- End diff --

Starting from this class, github does not highlight Java code. I remember 
@vdiravka had similar problem, it turned out that he copied code from text 
editor with some hidden symbols. Please check and fix.


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r161208998
  
--- Diff: common/src/test/java/org/apache/drill/test/DrillTest.java ---
@@ -54,8 +56,7 @@
   static MemWatcher memWatcher;
   static String className;
 
-  @Rule public final TestRule TIMEOUT = TestTools.getTimeoutRule(10);
-
+  @Rule public final TestRule TIMEOUT = new 
DisableOnDebug(TestTools.getTimeoutRule(100_000));
--- End diff --

Great enhancement!


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r161211784
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java ---
@@ -278,22 +273,62 @@ public void addDoubleMetrics(OperatorProfile.Builder 
builder) {
 }
   }
 
-  @Override
+  /**
+   * Set a stat to the specified long value. Creates the stat
+   * if the stat does not yet exist.
+   *
+   * @param metric the metric to update
+   * @param value the value to set
+   */
+
   public void addLongStat(MetricDef metric, long value){
 longMetrics.putOrAdd(metric.metricId(), value, value);
   }
 
-  @Override
+  @VisibleForTesting
+  public long getLongStat(MetricDef metric) {
+return longMetrics.get(metric.metricId());
--- End diff --

If metric is absent, will it return 0 or fail?


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1085#discussion_r161207650
  
--- Diff: 
common/src/main/java/org/apache/drill/common/exceptions/UserException.java ---
@@ -83,23 +83,17 @@ public static Builder memoryError() {
* The cause message will be used unless {@link 
Builder#message(String, Object...)} is called.
* If the wrapped exception is, or wraps, a user exception it will be 
returned by {@link Builder#build(Logger)}
* instead of creating a new exception. Any added context will be added 
to the user exception as well.
-   * 
-   * This exception, previously deprecated, has been repurposed to 
indicate unspecified
-   * errors. In particular, the case in which a lower level bit of code 
throws an
-   * exception other than UserException. The catching code then only knows 
"something went
-   * wrong", but not enough information to categorize the error.
-   * 
-   * System errors also indicate illegal internal states, missing 
functionality, and other
-   * code-related errors -- all of which "should never occur."
*
* @see 
org.apache.drill.exec.proto.UserBitShared.DrillPBError.ErrorType#SYSTEM
*
* @param cause exception we want the user exception to wrap. If cause 
is, or wrap, a user exception it will be
*  returned by the builder instead of creating a new user 
exception
* @return user exception builder
*
+   * @deprecated This method should never need to be used explicitly, 
unless you are passing the exception to the
+   * Rpc layer or UserResultListener.submitFailed()
*/
-
+  @Deprecated
--- End diff --

Deprecated means that we might remove it in future. But since you mention 
that it is still ok to use in certain cases, I am not sure that such annotation 
is appropriate.


---

[GitHub] drill pull request #1085: DRILL-6049: Misc. hygiene and code cleanup changes

[paul-rogers]

GitHub user paul-rogers opened a pull request:

https://github.com/apache/drill/pull/1085

DRILL-6049: Misc. hygiene and code cleanup changes

Collection of "hygiene" changes arising from the vector column writer 
project. This represents the set of changes outside of the core mechanism so 
that reviews can be done in multiple PRs rather than a single monster PR that 
mixes generic and core work.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/paul-rogers/drill DRILL-6049

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1085.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 #1085


commit be56804cfd68ba3a4645fdc526f06ee69b402f06
Author: Paul Rogers 
Date:   2017-12-21T07:59:06Z

DRILL-6049: Misc. hygiene and code cleanup changes




---