[GitHub] drill issue #1092: DRILL-6088: MainLoginPageModel errors out when http.auth....

[sohami]

Github user sohami commented on the issue:

https://github.com/apache/drill/pull/1092
  
@arina-ielchiieva - I have rebased the commit's on latest master and made 
the change as suggested. I have also made the MainLoginPageModel class public 
since it was causing the issue without that at runtime. Looks like during 
testing I kept it as public but later while opening the PR I might have changed 
it to package-private and was seeing issues today again during test.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162521845
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -86,37 +117,35 @@
  * Multiple threads of execution.
  * 
  */
-
 public class OperatorFixture extends BaseFixture implements AutoCloseable {
 
+  public OperatorContext operatorContext(PhysicalOperator config) {
+return new MockOperatorContext(context, allocator(), config);
+  }
+
   /**
* Builds an operator fixture based on a set of config options and 
system/session
* options.
*/
-
-  public static class OperatorFixtureBuilder
+  public static class Builder
--- End diff --

It's a nested class in the master branch currently 
https://github.com/apache/drill/blob/master/exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java
 . So I didn't change that in this PR. Did you want me to pull it out into a 
top level class as part of this PR?


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[paul-rogers]

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

https://github.com/apache/drill/pull/1045#discussion_r162518120
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java 
---
@@ -58,23 +55,24 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.Resources;
 
-public class PlanningBase extends ExecTest{
-  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(PlanningBase.class);
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
--- End diff --

Thanks!


---

[GitHub] drill pull request #1072: DRILL-5879: Improved SQL Pattern Contains Performa...

[paul-rogers]

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

https://github.com/apache/drill/pull/1072#discussion_r162517870
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/impl/TestSqlPatterns.java
 ---
@@ -446,6 +446,61 @@ public void testSqlPatternComplex() {
 assertEquals(1, sqlPatternComplex.match(0, byteBuffer.limit(), 
drillBuf)); // should match
   }
 
+  @Test
+  public void testSqlPatternContainsMUltipleMatchers() {
--- End diff --

If tests exist, and you have verified that each of the lengths is, in fact, 
tested by those tests, then I'm fine with adding a comment identifying the 
cases and where to find the existing tests. The point is to make sure all 
variations are fully tested -- one way or another.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[paul-rogers]

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

https://github.com/apache/drill/pull/1045#discussion_r162517423
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestSorter.java
 ---
@@ -59,7 +59,7 @@
 @Category(OperatorTest.class)
 public class TestSorter extends DrillTest {
 
-  public static OperatorFixture fixture;
+  public volatile static OperatorFixture fixture;
--- End diff --

Strange... So, if having volatile does solve a problem, just leave it in, 
but maybe add a comment that includes you note above so that we know what's 
what in the future.


---

[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...

[paul-rogers]

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

https://github.com/apache/drill/pull/1094#discussion_r162516548
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, 
AvaticaFactory factory,
 bit = new Drillbit(dConfig, serviceSet);
 bit.run();
   } catch (final UserException e) {
+cleanup();
 throw new SQLException(
 "Failure in starting embedded Drillbit: " + e.getMessage(),
 e);
   } catch (Exception e) {
+cleanup();
--- End diff --

One trick is this: used nested exceptions. The inner set "parses" the 
exception types, the outer does cleanup:

```
try {
  try {
// do something
  } catch (ExceptionA e) {
// Do something
  } catch (ExceptionB e) {
// Do something else
  ... }
 } catch (Throwable t) {
cleanup();
throw t;
 }
```


---

[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.


---

[jira] [Created] (DRILL-6099) Drill does not push limit past project (flatten) if it cannot be pushed into scan

[Gautam Kumar Parai (JIRA)]

Gautam Kumar Parai created DRILL-6099:
-

 Summary: Drill does not push limit past project (flatten) if it 
cannot be pushed into scan
 Key: DRILL-6099
 URL: https://issues.apache.org/jira/browse/DRILL-6099
 Project: Apache Drill
  Issue Type: Bug
Affects Versions: 1.12.0
Reporter: Gautam Kumar Parai
Assignee: Gautam Kumar Parai
 Fix For: 1.13.0


It would be useful to have pushdown occur past flatten(project). Here is an 
example to illustrate the issue:

{{explain plan without implementation for }}{{select name, flatten(categories) 
as category from dfs.`/tmp/t_json_20` LIMIT 1;}}

{{DrillScreenRel}}{{  }}

{{  DrillLimitRel(fetch=[1])}}{{    }}

{{    DrillProjectRel(name=[$0], category=[FLATTEN($1)])}}

{{      DrillScanRel(table=[[dfs, /tmp/t_json_20]], groupscan=[EasyGroupScan 
[selectionRoot=maprfs:/tmp/t_json_20, numFiles=1, columns=[`name`, 
`categories`], files=[maprfs:///tmp/t_json_20/0_0_0.json]]])}}

= 

Content of 0_0_0.json

=

{

  "name" : "Eric Goldberg, MD",

  "categories" : [ "Doctors", "Health & Medical" ]

} {

  "name" : "Pine Cone Restaurant",

  "categories" : [ "Restaurants" ]

} {

  "name" : "Deforest Family Restaurant",

  "categories" : [ "American (Traditional)", "Restaurants" ]

} {

  "name" : "Culver's",

  "categories" : [ "Food", "Ice Cream & Frozen Yogurt", "Fast Food", 
"Restaurants" ]

} {

  "name" : "Chang Jiang Chinese Kitchen",

  "categories" : [ "Chinese", "Restaurants" ]

} 

 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162513655
  
--- Diff: 
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveUtilities.java
 ---
@@ -288,7 +288,7 @@ public static void populateVector(final ValueVector 
vector, final DrillBuf manag
 }
   }
 
-  public static MajorType getMajorTypeFromHiveTypeInfo(final TypeInfo 
typeInfo, final OptionManager options) {
+  public static MajorType getMajorTypeFromHiveTypeInfo(final TypeInfo 
typeInfo, final OptionSet options) {
--- End diff --

I'll flip to using the OptionManager.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162513475
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
 ---
@@ -118,7 +118,7 @@ public IterOutcome innerNext() {
 } catch(IOException ex) {
   logger.error("Failure during query", ex);
   kill(false);
-  context.fail(ex);
+  context.getExecutorState().fail(ex);
--- End diff --

Sounds fun. Created https://issues.apache.org/jira/browse/DRILL-6098


---

[jira] [Created] (DRILL-6098) Make Drill Failure Handling Consistent

[Timothy Farkas (JIRA)]

Timothy Farkas created DRILL-6098:
-

 Summary: Make Drill Failure Handling Consistent
 Key: DRILL-6098
 URL: https://issues.apache.org/jira/browse/DRILL-6098
 Project: Apache Drill
  Issue Type: Improvement
Reporter: Timothy Farkas
Assignee: Timothy Farkas


As described by [~Paul.Rogers]
"

We have multiple ways of reporting errors:
 * Throw a UserException explaining the error
 * Throw an unchecked exception and and let the fragment executor guess the 
cause.
 * Return STOP
 * Tell the fragment executor to fail. (A we also required to return STOP?)
 * Return OUT_OF_MEMORY status

The proposal is to replace them all with a single solution: throw a 
UserException. Each operator catches other exceptions and translates them to 
UserException.

Java unwinds the stack just fine; no reason for us to write code to do it via 
STOP.

Then, the Fragment Executor calls close() on all operators. No reason to try to 
do this cleanup on STOP. (Even if we do, the lower-level operators won't have 
seen the STOP.)

Since failures are hard to test, and have cause no end of problems, having 
multiple ways to do the same thing is really not that helpful to Drill users.
"



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162512891
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
 ---
@@ -172,7 +172,7 @@ public RawFragmentBatch getNext() throws IOException {
 } catch (final InterruptedException e) {
 
   // We expect that the interrupt means the fragment is canceled or 
failed, so we should kill this buffer
-  if (!context.shouldContinue()) {
+  if (!context.getExecutorState().shouldContinue()) {
--- End diff --

ExecutorState is an interface. I removed those methods from the 
FragmentContext interface because it felt redundant to replicate the 
ExecutorState interface methods in the FragmentContext interface.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162512429
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java 
---
@@ -58,23 +55,24 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.Resources;
 
-public class PlanningBase extends ExecTest{
-  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(PlanningBase.class);
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
--- End diff --

https://issues.apache.org/jira/browse/DRILL-6097 will be the next step 
forward to removing our dependence on Mocking libraries :).


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162512220
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java 
---
@@ -58,23 +55,24 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.Resources;
 
-public class PlanningBase extends ExecTest{
-  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(PlanningBase.class);
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
--- End diff --

This test was previously using JMockit and I have simply replaced it with 
Mockito which is Eclipse friendly and a step forward for developers using 
Eclipse. So the amount of Mocking going on here is the same :) the only 
difference is that Eclipse users should now be able to run these tests. I have 
also completely removed mocking from some tests which no longer need it. As we 
take more steps to properly use interfaces for more classes, we can 
incrementally remove Mockito from even more tests. However, this is an 
incremental process and shouldn't be done all in one shot.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162511432
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java 
---
@@ -84,28 +82,17 @@ protected void testSqlPlan(String sqlCommands) throws 
Exception {
 systemOptions.init();
 @SuppressWarnings("resource")
 final UserSession userSession = 
UserSession.Builder.newBuilder().withOptionManager(systemOptions).build();
-final SessionOptionManager sessionOptions = (SessionOptionManager) 
userSession.getOptions();
+final SessionOptionManager sessionOptions = userSession.getOptions();
 final QueryOptionManager queryOptions = new 
QueryOptionManager(sessionOptions);
 final ExecutionControls executionControls = new 
ExecutionControls(queryOptions, DrillbitEndpoint.getDefaultInstance());
 
-new NonStrictExpectations() {
-  {
-dbContext.getMetrics();
-result = new MetricRegistry();
-dbContext.getAllocator();
-result = allocator;
-dbContext.getConfig();
-result = config;
-dbContext.getOptionManager();
-result = systemOptions;
-dbContext.getStoreProvider();
-result = provider;
-dbContext.getClasspathScan();
-result = scanResult;
-dbContext.getLpPersistence();
-result = logicalPlanPersistence;
-  }
-};
+when(dbContext.getMetrics()).thenReturn(new MetricRegistry());
+when(dbContext.getAllocator()).thenReturn(allocator);
+when(dbContext.getConfig()).thenReturn(config);
+when(dbContext.getOptionManager()).thenReturn(systemOptions);
+when(dbContext.getStoreProvider()).thenReturn(provider);
+when(dbContext.getClasspathScan()).thenReturn(scanResult);
+when(dbContext.getLpPersistence()).thenReturn(logicalPlanPersistence);
--- End diff --

I completely agree with this in the long term. Since this code deals with 
the QueryContext I did not dive into creating an interface for the QueryContext 
and a Mock implementation of the interface. I want to limit the scope of this 
PR since it is already quite large, but I have created a follow up ticket to 
handle the mocking of the QueryContext correctly 
https://issues.apache.org/jira/browse/DRILL-6097 . As an additional note, the 
only reason why this code was changed was to take a step towards removing 
JMockit which does not play nice with Eclipse as you have noticed. Removing 
JMockit and replacing it with Mockito was the easiest thing to do at this 
point. Once this change goes in we can revisit the QueryContext in DRILL-6097.


---

[jira] [Created] (DRILL-6097) Create an interface for the QueryContext

[Timothy Farkas (JIRA)]

Timothy Farkas created DRILL-6097:
-

 Summary: Create an interface for the QueryContext
 Key: DRILL-6097
 URL: https://issues.apache.org/jira/browse/DRILL-6097
 Project: Apache Drill
  Issue Type: Improvement
 Environment: Currently the QueryContext does not implement an 
interface and the concrete class is passed around everywhere. Additionally 
Mockito is used in tests to mock it. Ideally we would make the QueryContext 
implement an interface and create a mock implementation of it that is used in 
the tests, just like what we did for the FragmentContext.
Reporter: Timothy Farkas
Assignee: Timothy Farkas






--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[GitHub] drill issue #1072: DRILL-5879: Improved SQL Pattern Contains Performance

[priteshm]

Github user priteshm commented on the issue:

https://github.com/apache/drill/pull/1072
  
@paul-rogers Is this ready for commit?


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162501868
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestSorter.java
 ---
@@ -59,7 +59,7 @@
 @Category(OperatorTest.class)
 public class TestSorter extends DrillTest {
 
-  public static OperatorFixture fixture;
+  public volatile static OperatorFixture fixture;
--- End diff --

I was running into an issue where the fixture variable was consistently 
null when the test ran. This seemed impossible and I hypothesized that JUnit is 
secretly using two threads, one to call the static initializers and then 
another to actually execute the test methods. So I made the variable volatile 
and the issue went away. I agree this is weird and it raises the question why 
other tests don't have the same issue. I'll remove the volatile for now, if the 
bizarre issue surfaces again, I will get more details and post them here.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162499201
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestAffinityCalculator.java
 ---
@@ -21,18 +21,14 @@
 
 import org.apache.drill.exec.ExecTest;
 import org.apache.drill.exec.proto.CoordinationProtos;
-import org.apache.drill.exec.store.parquet.ParquetGroupScan;
 import org.apache.hadoop.fs.BlockLocation;
 import org.junit.Test;
 
 import com.google.common.collect.ImmutableRangeMap;
 import com.google.common.collect.Range;
 
 public class TestAffinityCalculator extends ExecTest {
-  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(TestAffinityCalculator.class);
-
-  String port = "1234";
-  final String path = "path";
+  private final String port = "1234";
--- End diff --

 - Two tests were blatantly commented out.
   - testSetEndpointBytes
   - testApplyAssignments
 - Several methods were unused according to IntelliJ and the java compiler.
   - buildRowGroups
   - buildEndpoints
   - buildBlockLocations2

After removing the unused code there is shockingly almost nothing left. 
There are basically no functioning unit tests for the AffinityCalculator.


---

[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[paul-rogers]

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

https://github.com/apache/drill/pull/1045#discussion_r162497247
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -86,37 +117,35 @@
  * Multiple threads of execution.
  * 
  */
-
 public class OperatorFixture extends BaseFixture implements AutoCloseable {
 
+  public OperatorContext operatorContext(PhysicalOperator config) {
+return new MockOperatorContext(context, allocator(), config);
+  }
+
   /**
* Builds an operator fixture based on a set of config options and 
system/session
* options.
*/
-
-  public static class OperatorFixtureBuilder
+  public static class Builder
--- End diff --

Yes, of course. But one of the changes was to pull this out to a new 
top-level class. In that case, "Builder" by itself is rather generic.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162496772
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -86,37 +117,35 @@
  * Multiple threads of execution.
  * 
  */
-
 public class OperatorFixture extends BaseFixture implements AutoCloseable {
 
+  public OperatorContext operatorContext(PhysicalOperator config) {
+return new MockOperatorContext(context, allocator(), config);
+  }
+
   /**
* Builds an operator fixture based on a set of config options and 
system/session
* options.
*/
-
-  public static class OperatorFixtureBuilder
+  public static class Builder
--- End diff --

There are no collisions. Java has a nice syntax for handling names like 
this. Specifically you can use this name for the class 
**OperatorFixture.Builder** in all your variable declarations. This removes the 
redundancy of prefixing the name of an inner class with the name of the outer 
class. 


---

[jira] [Created] (DRILL-6096) Provide mechanisms to specify field delimiters and quoted text for TextRecordWriter

[Kunal Khatua (JIRA)]

Kunal Khatua created DRILL-6096:
---

 Summary: Provide mechanisms to specify field delimiters and quoted 
text for TextRecordWriter
 Key: DRILL-6096
 URL: https://issues.apache.org/jira/browse/DRILL-6096
 Project: Apache Drill
  Issue Type: Bug
  Components: Storage - Text  CSV
Affects Versions: 1.12.0
Reporter: Kunal Khatua


Currently, there is no way for a user to specify the field delimiter for the 
writing records as a text output. Further more, if the fields contain the 
delimiter, we have no mechanism of specifying quotes.

By default, quotes should be used to enclose non-numeric fields being written.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162495937
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -127,20 +156,26 @@ public OperatorFixture build() {
* uses the same code generation mechanism as the full Drill, but
* provide test-specific versions of various other services.
*/
-
-  public static class TestFragmentContext extends BaseFragmentContext {
-
+  public static class MockFragmentContext extends BaseFragmentContext {
--- End diff --

I like prefixing Mock classes with **Mock** instead of **Test**.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162495701
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -351,8 +603,110 @@ public OperatorStats getStats() {
 }
   }
 
-  public OperatorContext operatorContext(PhysicalOperator config) {
-return new TestOperatorContext(context, allocator(), config, stats);
+  public static class MockPhysicalOperator implements PhysicalOperator {
--- End diff --

Thanks for catching this. This class is left over from some previous work 
and is not used. I've deleted it.


---

[GitHub] drill pull request #1045: DRILL-5730 Test Mocking Improvements

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1045#discussion_r162495593
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/OperatorFixture.java ---
@@ -175,19 +210,189 @@ public DrillConfig getConfig() {
 }
 
 @Override
-public DrillbitContext getDrillbitContext() {
-  throw new UnsupportedOperationException("Drillbit context not 
available for operator unit tests");
+public ExecutorService getScanDecodeExecutor() {
+  return null;
+}
+
+@Override
+public ExecutorService getScanExecutor() {
+  return null;
+}
+
+@Override
+public ExecutorService getExecutor() {
+  return null;
+}
+
+@Override
+public ExecutorState getExecutorState() {
+  return executorState;
+}
+
+@Override
+public BufferAllocator getNewChildAllocator(String operatorName, int 
operatorId,
+long initialReservation, 
long maximumReservation) {
+  return allocator.newChildAllocator(
+"op:" + operatorId + ":" + operatorName,
+initialReservation,
+maximumReservation);
 }
 
 @Override
-protected CodeCompiler getCompiler() {
+public ExecProtos.FragmentHandle getHandle() {
+  return ExecProtos.FragmentHandle.newBuilder().build();
+}
+
+@Override
+public BufferAllocator getAllocator() {
+  return allocator;
+}
+
+@Override
+public OperatorContext newOperatorContext(PhysicalOperator popConfig) {
+  return mockOperatorContext;
+}
+
+@Override
+public OperatorContext newOperatorContext(PhysicalOperator popConfig, 
OperatorStats stats) {
+  return mockOperatorContext;
+}
+
+@Override
+public SchemaPlus getFullRootSchema() {
+  return null;
+}
+
+@Override
+public String getQueryUserName() {
+  return null;
+}
+
+@Override
+public String getFragIdString() {
+  return null;
+}
+
+@Override
+public CodeCompiler getCompiler() {
return compiler;
 }
 
 @Override
 protected BufferManager getBufferManager() {
   return bufferManager;
 }
+
+@Override
+public void close() {
+  bufferManager.close();
+}
+
+@Override
+public ContextInformation getContextInformation() {
+  return null;
+}
+
+@Override
+public PartitionExplorer getPartitionExplorer() {
+  return null;
+}
+
+@Override
+public ValueHolder getConstantValueHolder(String value, 
TypeProtos.MinorType type, Function holderInitializer) {
+  return null;
+}
+  }
+
+  public static class MockExecutorFragmentContext extends 
MockFragmentContext implements ExecutorFragmentContext {
+
+public MockExecutorFragmentContext(DrillConfig config, OptionManager 
optionManager, BufferAllocator allocator) {
+  super(config, optionManager, allocator);
+}
+
+@Override
+public PhysicalPlanReader getPlanReader() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public ClusterCoordinator getClusterCoordinator() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public CoordinationProtos.DrillbitEndpoint getForemanEndpoint() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public CoordinationProtos.DrillbitEndpoint getEndpoint() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public Collection getBits() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public OperatorCreatorRegistry getOperatorCreatorRegistry() {
+  return null;
+}
+
+@Override
+public void setBuffers(IncomingBuffers buffers) {
+
+}
+
+@Override
+public Set> getUserConnections() {
+  return null;
+}
+
+@Override
+public QueryProfileStoreContext getProfileStoreContext() {
+  return null;
+}
+
+@Override
+public void waitForSendComplete() {
+  throw new UnsupportedOperationException();
+}
+
+@Override
+public AccountingDataTunnel 
getDataTunnel(CoordinationProtos.DrillbitEndpoint endpoint) {
--- End diff --

Agreed. This is 

[GitHub] drill pull request #1084: DRILL-5868: Support SQL syntax highlighting of que...

[kkhatua]

Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1084#discussion_r162478583
  
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -77,16 +84,17 @@ table.sortable thead .sorting_desc { background-image: 
url("/static/img/black-de
   
 
 <#if model.isOnlyImpersonationEnabled()>
+  ${model.getProfile().query}
+  
   
 User Name
 
   
 
 
 
-  
-${model.getProfile().query}
-  
+  ${model.getProfile().query}
+  
--- End diff --

Patched and Verified that with Impersonation enabled, there is only one 
editor for the profile page.


---

[GitHub] drill issue #570: DRILL-4834 decimal implementation is vulnerable to overflo...

[daveoshinsky]

Github user daveoshinsky commented on the issue:

https://github.com/apache/drill/pull/570
  
The new VARDECIMAL one-size-fits-all decimal type, which this PR 
implements, will now be incorporated into the following new JIRA with 
additional changes and fixes for Drill 1.13:
https://issues.apache.org/jira/browse/DRILL-6094

So, development on this PR will now cease.  But VARDECIMAL lives on
Dave Oshinsky


---

Decimal data type enhancements

[Vova Vysotskyi]

Hi all,

I am going to work on improving decimal data types.
I have created Jira DRILL-6094
 and design document

where analyzed the current state of Decimal data types and how it may be
improved.

-- 
Kind regards,
Volodymyr Vysotskyi

[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15

[vvysotskyi]

Github user vvysotskyi commented on the issue:

https://github.com/apache/drill/pull/1066
  
FYI: changes in pom files:
We observed issue with Avatica similar to the issue described in 
https://issues.apache.org/jira/browse/CALCITE-1694. Therefore we had to make 
changes in our pom files similar to the changes, made in CALCITE-1694 to fix 
NoClassDefFoundError.


---

[GitHub] drill pull request #1084: DRILL-5868: Support SQL syntax highlighting of que...

[kkhatua]

Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/1084#discussion_r162384019
  
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -77,16 +84,17 @@ table.sortable thead .sorting_desc { background-image: 
url("/static/img/black-de
   
 
 <#if model.isOnlyImpersonationEnabled()>
+  ${model.getProfile().query}
+  
   
 User Name
 
   
 
 
 
-  
-${model.getProfile().query}
-  
+  ${model.getProfile().query}
+  
--- End diff --

Let me take a look. This might have slipped past during a merge conflict. 


---

[GitHub] drill pull request #1092: DRILL-6088: MainLoginPageModel errors out when htt...

[arina-ielchiieva]

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

https://github.com/apache/drill/pull/1092#discussion_r162331112
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java
 ---
@@ -132,24 +133,26 @@ public Viewable getMainLoginPage(@Context 
HttpServletRequest request, @Context H
@Context SecurityContext sc, @Context 
UriInfo uriInfo,

@QueryParam(WebServerConstants.REDIRECT_QUERY_PARM) String redirect) throws 
Exception {
 updateSessionRedirectInfo(redirect, request);
-final DrillConfig drillConfig = workManager.getContext().getConfig();
-MainLoginPageModel model = new MainLoginPageModel(null, drillConfig);
+final MainLoginPageModel model = new MainLoginPageModel(null);
 return ViewableWithPermissions.createMainLoginPage(model);
   }
 
-  private class MainLoginPageModel {
+  @VisibleForTesting
+  class MainLoginPageModel {
 
 private final String error;
 
 private final boolean authEnabled;
 
+private final DrillConfig config;
--- End diff --

It looks like you are using config only in constructor, so it can not store 
it in class.


---

[GitHub] drill pull request #1084: DRILL-5868: Support SQL syntax highlighting of que...

[arina-ielchiieva]

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

https://github.com/apache/drill/pull/1084#discussion_r162329512
  
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -77,16 +84,17 @@ table.sortable thead .sorting_desc { background-image: 
url("/static/img/black-de
   
 
 <#if model.isOnlyImpersonationEnabled()>
+  ${model.getProfile().query}
+  
   
 User Name
 
   
 
 
 
-  
-${model.getProfile().query}
-  
+  ${model.getProfile().query}
+  
--- End diff --

It looks like you have the same code on lines 87-88. These code lines are 
included when only impersonation is enabled. So if I am not mistaken, you'll 
end up with two query editors when only impersonation is enabled. Please check.


---

[GitHub] drill issue #1095: DRILL-6085: Fixed spontaneous vm exits on Travis.

[arina-ielchiieva]

Github user arina-ielchiieva commented on the issue:

https://github.com/apache/drill/pull/1095
  
+1


---

[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...

[arina-ielchiieva]

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

https://github.com/apache/drill/pull/1094#discussion_r162311140
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, 
AvaticaFactory factory,
 bit = new Drillbit(dConfig, serviceSet);
 bit.run();
   } catch (final UserException e) {
+cleanup();
 throw new SQLException(
 "Failure in starting embedded Drillbit: " + e.getMessage(),
 e);
   } catch (Exception e) {
+cleanup();
--- End diff --

What if you use flag `doCleanup` which will be true by default and after 
`this.client.connect(connect, info);` succeedes, it would be set to false. In 
finally block you'll do clean up only if flag is true?


---

[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15

[vvysotskyi]

Github user vvysotskyi commented on the issue:

https://github.com/apache/drill/pull/1066
  
@amansinha100, regarding HashAggregate OOM related change, it was done in 
the scope of this pull request since with new Calcite a physical plan for the 
query was changed to the correct one but it caused an infinite loop in HashAgg 
operator. Therefore I made these changes in order to prevent this infinite loop 
for the queries that previously worked. 
I still think that it is better to keep this change in this PR.


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162292084
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestMergeJoinAdvanced.java
 ---
@@ -38,13 +37,16 @@
 import java.util.Random;
 
 @Category(OperatorTest.class)
-public class TestMergeJoinAdvanced extends BaseTestQuery {
+public class TestMergeJoinAdvanced extends JoinTestBase {
   private static final String LEFT = "merge-join-left.json";
   private static final String RIGHT = "merge-join-right.json";
+  private static String mjpattern = "MergeJoin";
--- End diff --

Make it as a constant --> private static final String MJ_PATTERN = 
"MergeJoin";


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162292571
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestNestedLoopJoin.java
 ---
@@ -19,19 +19,17 @@
 package org.apache.drill.exec.physical.impl.join;
 
 import org.apache.drill.categories.OperatorTest;
-import org.apache.drill.PlanTestBase;
 import org.apache.drill.common.exceptions.UserRemoteException;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-
 import java.nio.file.Paths;
-
+import org.apache.drill.exec.planner.physical.PlannerSettings;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertThat;
 
 @Category(OperatorTest.class)
-public class TestNestedLoopJoin extends PlanTestBase {
+public class TestNestedLoopJoin extends JoinTestBase {
 
   private static String nlpattern = "NestedLoopJoin";
--- End diff --

Can you edit this as well? --> final, NLJ_PATTERN


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162293231
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/HashJoinBatch.java
 ---
@@ -535,4 +541,8 @@ public void close() {
 }
 super.close();
   }
+
+  private boolean isFurtherProcessingRequired(IterOutcome upStream) {
--- End diff --

It will be good to have here a small java-doc


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162289830
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/JoinTestBase.java
 ---
@@ -0,0 +1,70 @@
+/*
+ * 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.impl.join;
+
+import org.apache.drill.categories.OperatorTest;
+import org.apache.drill.PlanTestBase;
+import org.apache.drill.common.exceptions.UserRemoteException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.nio.file.Paths;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertThat;
+
+
+@Category(OperatorTest.class)
+public class JoinTestBase extends PlanTestBase {
+
+  private static final String testEmptyJoin = "select count(*) as cnt from 
cp.`employee.json` emp %s join dfs.`dept.json` " +
+  "as dept on dept.manager = emp.`last_name`";
+
+  /**
+   * This function runs a join query with one of the table generated as an
+   * empty json file.
+   * @param testDir in which the empty json file is generated.
+   * @param joinType to be executed.
+   * @param joinPattern to look for the pattern in the successful run.
+   * @param result number of the output rows.
+   */
+  public void testJoinWithEmptyFile(File testDir, String joinType,
+ String joinPattern, long result) throws Exception 
{
+buildFile("dept.json", new String[0], testDir);
+String query = String.format(testEmptyJoin, joinType);
+testPlanMatchingPatterns(query, new String[]{joinPattern}, new 
String[]{});
+testBuilder()
+.sqlQuery(query)
+.unOrdered()
+.baselineColumns("cnt")
+.baselineValues(result)
+.build().run();
+  }
+
+  private void buildFile(String fileName, String[] data, File testDir) 
throws IOException {
+try(PrintWriter out = new PrintWriter(new FileWriter(new File(testDir, 
fileName {
+  for (String line : data) {
+out.println(line);
+  }
+}
+  }
+}
--- End diff --

Put a new line in the end of file


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162292685
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoinAdvanced.java
 ---
@@ -19,20 +19,22 @@
 package org.apache.drill.exec.physical.impl.join;
 
 
-import org.apache.drill.test.BaseTestQuery;
 import org.apache.drill.categories.OperatorTest;
 import org.apache.drill.categories.UnlikelyTest;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-
 import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileWriter;
 
+
 @Category(OperatorTest.class)
-public class TestHashJoinAdvanced extends BaseTestQuery {
+public class TestHashJoinAdvanced extends JoinTestBase {
+
+  private static String hjpattern = "HashJoin";
--- End diff --

--> final, HJ_PATTERN


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162293578
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java
 ---
@@ -228,4 +228,20 @@ public WritableBatch getWritableBatch() {
   public VectorContainer getOutgoingContainer() {
 throw new UnsupportedOperationException(String.format(" You should not 
call getOutgoingContainer() for class %s", this.getClass().getCanonicalName()));
   }
+
+  public void drainStream(IterOutcome stream, int input, RecordBatch 
batch) {
--- End diff --

Please add java-doc for this method


---

[GitHub] drill pull request #1059: DRILL-5851: Empty table during a join operation wi...

[vdiravka]

Github user vdiravka commented on a diff in the pull request:

https://github.com/apache/drill/pull/1059#discussion_r162294377
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/JoinTestBase.java
 ---
@@ -0,0 +1,70 @@
+/*
+ * 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.impl.join;
+
+import org.apache.drill.categories.OperatorTest;
+import org.apache.drill.PlanTestBase;
+import org.apache.drill.common.exceptions.UserRemoteException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.nio.file.Paths;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertThat;
+
+
+@Category(OperatorTest.class)
+public class JoinTestBase extends PlanTestBase {
+
+  private static final String testEmptyJoin = "select count(*) as cnt from 
cp.`employee.json` emp %s join dfs.`dept.json` " +
+  "as dept on dept.manager = emp.`last_name`";
+
+  /**
+   * This function runs a join query with one of the table generated as an
--- End diff --

function -> method


---

[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...

[milindt]

Github user milindt commented on a diff in the pull request:

https://github.com/apache/drill/pull/1094#discussion_r162286759
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, 
AvaticaFactory factory,
 bit = new Drillbit(dConfig, serviceSet);
 bit.run();
   } catch (final UserException e) {
+cleanup();
 throw new SQLException(
 "Failure in starting embedded Drillbit: " + e.getMessage(),
 e);
   } catch (Exception e) {
+cleanup();
--- End diff --

We don't want clean up to run on successful connection though, we just want 
to cover all the exceptions. On a successful connection, the "Curator" threads 
will be created but the number of threads will be proportional to number of 
connections, which can be be controlled by using a connection pool, hence it 
won't cause this issue. 

On the other hand,  calling cleanup on a successful connection may 
terminate that connection.  Thoughts?


---

[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...

[arina-ielchiieva]

Github user arina-ielchiieva commented on the issue:

https://github.com/apache/drill/pull/1094
  
@milindt release 1.12.0 is out. Regarding this fix, once you address code 
review comments it will be added in 1.13.0.


---

[GitHub] drill pull request #1094: DRILL-6090: While connecting to drill-bits using J...

[arina-ielchiieva]

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

https://github.com/apache/drill/pull/1094#discussion_r162282830
  
--- Diff: 
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -132,10 +132,12 @@ protected DrillConnectionImpl(DriverImpl driver, 
AvaticaFactory factory,
 bit = new Drillbit(dConfig, serviceSet);
 bit.run();
   } catch (final UserException e) {
+cleanup();
 throw new SQLException(
 "Failure in starting embedded Drillbit: " + e.getMessage(),
 e);
   } catch (Exception e) {
+cleanup();
--- End diff --

I suppose you can use finally block to perform clean up instead of 
repeating `cleanup` method call after each cached exception.


---

[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...

[milindt]

Github user milindt commented on the issue:

https://github.com/apache/drill/pull/1094
  
@paul-rogers can you please review this fix?


---

[GitHub] drill issue #1094: DRILL-6090: While connecting to drill-bits using JDBC Dri...

[milindt]

Github user milindt commented on the issue:

https://github.com/apache/drill/pull/1094
  
Details in [DRILL-6090](https://issues.apache.org/jira/browse/DRILL-6090)


---

[GitHub] drill issue #1093: DRILL-6093 : Account for simple columns in project cpu co...

[gparai]

Github user gparai commented on the issue:

https://github.com/apache/drill/pull/1093
  
@amansinha100 I have addressed your comments.


---