Build failed in Jenkins: drill-scm #934

[Apache Jenkins Server]

See 

Changes:

[parthc] DRILL-6129: Fixed query failure due to nested column data type change

[parthc] DRILL-6138: Move RecordBatchSizer to org.apache.drill.exec.record

[parthc] DRILL-6144: Make direct memory amount configurable in tests.

--
[...truncated 428.84 KB...]
at 
org.apache.maven.lifecycle.internal.MojoExecutor.ensureDependenciesAreResolved(MojoExecutor.java:257)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:200)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
at 
org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
at 
org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
at 
org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:80)
at 
org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:51)
at 
org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:307)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:193)
at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:106)
at org.apache.maven.cli.MavenCli.execute(MavenCli.java:862)
at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:286)
at org.apache.maven.cli.MavenCli.main(MavenCli.java:197)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
at 
org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
Downloading: 
http://conjars.org/repo/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
 Downloading: 
http://repository.mapr.com/maven/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
 Downloading: 
http://repo.dremio.com/release/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
 Downloading: 
http://repository.mapr.com/nexus/content/repositories/drill/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
 Downloading: 
https://repo.maven.apache.org/maven2/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
3/5 KB   5/5 KBDownloaded: 
https://repo.maven.apache.org/maven2/org/glassfish/jersey/media/jersey-media-json-jackson/2.8/jersey-media-json-jackson-2.8.pom
 (5 KB at 303.8 KB/sec)
Downloading: 
http://conjars.org/repo/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
 Downloading: 
http://repository.mapr.com/maven/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
 Downloading: 
http://repo.dremio.com/release/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
 Downloading: 
http://repository.mapr.com/nexus/content/repositories/drill/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
 Downloading: 
https://repo.maven.apache.org/maven2/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
3/4 KB   4/4 KBDownloaded: 
https://repo.maven.apache.org/maven2/com/fasterxml/jackson/jaxrs/jackson-jaxrs-json-provider/2.7.9/jackson-jaxrs-json-provider-2.7.9.pom
 (4 KB at 247.3 KB/sec)
Downloading: 
http://conjars.org/repo/com/fasterxml/jackson/jaxrs/jackson-jaxrs-providers/2.7.9/jackson-jaxrs-providers-2.7.9.pom
 Downloading: 
http://repository.mapr.com/maven/com/fasterxml/jackson/jaxrs/jackson-jaxrs-providers/2.7.9/jackson-jaxrs-providers-2.7.9.pom
 Downloading: 
http://repo.dremio.com/release/com/fasterxml/jackson/jaxrs/jackson-jaxrs-providers/2.7.9/jackson-jaxrs-providers-2.7.9.pom
 Downloading: 
http://repository.mapr.com/nexus/content/repositories/drill/com/fasterxml/jackson/jaxrs/jackson-jaxrs-providers/2.7.9/jackson-jaxrs-providers-2.7.9.pom
 Downloading: 

[GitHub] drill pull request #1106: DRILL-6129: Fixed query failure due to nested colu...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] drill pull request #1115: DRILL-6138: Move RecordBatchSizer to org.apache.dr...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] drill pull request #1118: DRILL-6144: Make directMemory amount configurable ...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167283942
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
+  protected static final int MIN_NUM_ROWS = 1;
+
+  public void update(int inputIndex) {};
+
+  public void update() {};
+
+  public int getOutputRowCount() {
+return outputRowCount;
+  }
+
+  /**
+   * Given batchSize and rowWidth, this will set output rowCount taking 
into account
+   * the min and max that is allowed.
+   */
+  public void setOutputRowCount(long batchSize, int rowWidth) {
+this.outputRowCount = Math.min(ValueVector.MAX_ROW_COUNT,
+  
Math.max(RecordBatchSizer.safeDivide(batchSize/WORST_CASE_FRAGMENTATION_FACTOR, 
rowWidth), MIN_NUM_ROWS));
--- End diff --

I suspect the math may be off here. `batchSize` is the actual memory for 
the batch? If so, then it already includes actual internal fragmentation which, 
depending on the source, can be 25% (for "good" operators) to 50% (if from the 
network) to 95% (pathological Parquet cases.) Using the fragmentation factor on 
this value gets us even more lost.

A better way is to do:

```
rawBatchSize = rowWidth * rowCount;
actualBatchSize = rowBatchSize * FRAG_FACTOR;
```

Or to compute the output size:

```
targetOutputSize = // some value you set
targetDataSize = targetOutputSize / FRAG_FACTOR
targetRowCount = targetDataSize / rowWidth
```


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167284434
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
+  protected static final int MIN_NUM_ROWS = 1;
+
+  public void update(int inputIndex) {};
+
+  public void update() {};
+
+  public int getOutputRowCount() {
+return outputRowCount;
+  }
+
+  /**
+   * Given batchSize and rowWidth, this will set output rowCount taking 
into account
+   * the min and max that is allowed.
+   */
+  public void setOutputRowCount(long batchSize, int rowWidth) {
+this.outputRowCount = Math.min(ValueVector.MAX_ROW_COUNT,
+  
Math.max(RecordBatchSizer.safeDivide(batchSize/WORST_CASE_FRAGMENTATION_FACTOR, 
rowWidth), MIN_NUM_ROWS));
--- End diff --

Should this method use the one below to set the limits?


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167281940
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
--- End diff --

Maybe put the contestants at the top? Then, set `outputRowCount = 
MAX_NUM_ROWS`.


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167146010
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/spill/RecordBatchSizer.java
 ---
@@ -137,10 +137,10 @@ public ColumnSize(ValueVector v, String prefix) {
   case MAP:
   case UNION:
 // No standard size for Union type
-dataSize = v.getPayloadByteCount(valueCount);
+dataSize = valueCount == 0 ? 0 : v.getPayloadByteCount(valueCount);
--- End diff --

Can the 0-handling be pushed into the `getPayloadByteCount` method? Seems 
error-prone and messy to sprinkle the 0-checks everywhere.


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167146312
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/spill/RecordBatchSizer.java
 ---
@@ -311,8 +311,8 @@ public static ColumnSize getColumn(ValueVector v, 
String prefix) {
 
   public RecordBatchSizer(RecordBatch batch) {
 this(batch,
- (batch.getSchema().getSelectionVectorMode() == 
BatchSchema.SelectionVectorMode.TWO_BYTE) ?
- batch.getSelectionVector2() : null);
+  (batch.getSchema() == null ? null : 
(batch.getSchema().getSelectionVectorMode() == 
BatchSchema.SelectionVectorMode.TWO_BYTE ?
--- End diff --

Does it even make sense to call the sizer if there is no schema? If there 
is no schema, there should be no vectors, so nothing to size.


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167284278
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
+  protected static final int MIN_NUM_ROWS = 1;
+
+  public void update(int inputIndex) {};
+
+  public void update() {};
+
+  public int getOutputRowCount() {
+return outputRowCount;
+  }
+
+  /**
+   * Given batchSize and rowWidth, this will set output rowCount taking 
into account
+   * the min and max that is allowed.
+   */
+  public void setOutputRowCount(long batchSize, int rowWidth) {
+this.outputRowCount = Math.min(ValueVector.MAX_ROW_COUNT,
+  
Math.max(RecordBatchSizer.safeDivide(batchSize/WORST_CASE_FRAGMENTATION_FACTOR, 
rowWidth), MIN_NUM_ROWS));
+  }
+
+  /**
+   * This will set rowCount taking into account the min and max that is 
allowed.
+   */
+  public int setOutputRowCount(int rowCount) {
--- End diff --

The `set` name is a misnomer. We are not setting anything. We are 
computing. And, since we don't reference fields, and don't change state, this 
can be `static`.


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[paul-rogers]

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

https://github.com/apache/drill/pull/1107#discussion_r167282219
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
+  protected static final int MIN_NUM_ROWS = 1;
+
+  public void update(int inputIndex) {};
+
+  public void update() {};
+
+  public int getOutputRowCount() {
+return outputRowCount;
+  }
+
+  /**
+   * Given batchSize and rowWidth, this will set output rowCount taking 
into account
+   * the min and max that is allowed.
+   */
+  public void setOutputRowCount(long batchSize, int rowWidth) {
--- End diff --

We probably never want a batch size larger than 2 GB.


---

[GitHub] drill issue #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a SystemOp...

[priteshm]

Github user priteshm commented on the issue:

https://github.com/apache/drill/pull/1119
  
@arina-ielchiieva is on vacation. @vrozov, @Ben-Zvi  can you take a look?


---

Re: Batch Sizing for Parquet Flat Reader

[salim achouche]

Thank you Parth for providing feedback; please find my answers below:

I have created Apache JIRA DRILL-6147
 for tracking
this improvement.

>  2) Not sure where you were going with the predicate pushdown section and
how it pertains to your proposed batch sizing.

Predicate push down was part of the Design Considerations section; the
intent is that the design should be able to handle future use-cases such as
push down. Notice how the Page based Statistical Approach will not work
well with predicate push down as one single batch can span many pages per
column.

> 3) Assuming that you go with the average batch size calculation approach,
are you proposing to have a Parquet scan specific overflow implementation?
Or are you planning to leverage the ResultSet loader mechanism? If you plan
to use the latter, it will need to be enhanced to handle a bulk chunk as
opposed to a single value at a time. If not using the ResultSet loader
mechanism, why not (you would be reinventing the wheel) ?

Padma Penumarthy and I are currently working on the batch sizing
functionality and selected few TPCH queries to show case end-to-end use
cases. Immediately after this task, I'll be working on enhancing the new
framework to support columnar processing and as such retrofit DRILL-6147
implementation as part of the new framework. So essentially we want to make
progress in both fronts so that a) OOM conditions are minimized as soon as
possible and b) the new Reader framework is applied to all readers and
operators is rolled out in the next few releases.

> Also note that memory allocations by Netty greater than the 16MB chunk
size
are returned to the OS when the memory is free'd. Both this document and
the original document on memory fragmentation state incorrectly that such
memory is not released back to the OS. A quick thought experiment - where
does this memory go if it is not released back to the OS?

I have the same understanding as you:
- I think Paul meant that 16 MB blocks are not released to the OS (cached
within Netty)
- Many memory allocators exhibit the same behavior as the release mechanism
is slow (heuristics used to decide when to release so to balance between
performance and resource usage)
- Basically, if Drill holds a large count of 16 MB blocks, than a 32 MB, 64
MB , etc memory allocation might fail since
  *  none of the Netty allocated blocks can satisfy the new request
  *  a new OS allocation will take Drill beyond the maximum direct memory


On Fri, Feb 9, 2018 at 4:08 AM, Parth Chandra  wrote:

> Is there a JIRA for this? Would be useful to capture the comments in the
> JIRA. Note that the document itself is not comment-able as it is shared
> with view-only permissions.
>
> Some thoughts in no particular order-
> 1) The Page based statistical approach is likely to run into trouble with
> the encoding used for Parquet fields especially RLE which drastically
> changes the size of the field. So pageSize/numValues is going to be wildly
> inaccurate with RLE.
> 2) Not sure where you were going with the predicate pushdown section and
> how it pertains to your proposed batch sizing.
> 3) Assuming that you go with the average batch size calculation approach,
> are you proposing to have a Parquet scan specific overflow implementation?
> Or are you planning to leverage the ResultSet loader mechanism? If you plan
> to use the latter, it will need to be enhanced to handle a bulk chunk as
> opposed to a single value at a time. If not using the ResultSet loader
> mechanism, why not (you would be reinventing the wheel) ?
> 4) Parquet page level stats are probably not reliable. You can assume page
> size (compressed/uncompressed) and value count are accurate, but nothing
> else.
>
> Also note that memory allocations by Netty greater than the 16MB chunk size
> are returned to the OS when the memory is free'd. Both this document and
> the original document on memory fragmentation state incorrectly that such
> memory is not released back to the OS. A quick thought experiment - where
> does this memory go if it is not released back to the OS?
>
>
>
> On Fri, Feb 9, 2018 at 7:12 AM, salim achouche 
> wrote:
>
> > The following document
> >  > 9RwG4h0sI81KI5ZEvJ7HzgClCUFpB5WE/edit?ts=5a793606#>
> > describes
> > a proposal for enforcing batch sizing constraints (count and memory)
> within
> > the Parquet Reader (Flat Schema). Please feel free to take a look and
> > provide feedback.
> >
> > Thanks!
> >
> > Regards,
> > Salim
> >
>

[GitHub] drill pull request #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a S...

[vrozov]

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

https://github.com/apache/drill/pull/1119#discussion_r167312527
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -413,6 +413,7 @@ drill.exec.options: {
 # to start at least 2 partitions then HashAgg fallbacks to this case. 
It can be
 # enabled by setting this flag to true. By default it's set to false 
such that
 # query will fail if there is not enough memory
+drill.exec.rpc.fragrunner.timeout: 3,
--- End diff --

The value looks a little bit high and it needs to be moved either below 
`drill.exec.hashagg.fallback.enabled` or above the preceding comment, otherwise 
LGTM.


---

[jira] [Created] (DRILL-6147) Limit batch size for Flat Parquet Reader

[salim achouche (JIRA)]

salim achouche created DRILL-6147:
-

 Summary: Limit batch size for Flat Parquet Reader
 Key: DRILL-6147
 URL: https://issues.apache.org/jira/browse/DRILL-6147
 Project: Apache Drill
  Issue Type: Improvement
  Components: Storage - Parquet
Reporter: salim achouche
Assignee: salim achouche
 Fix For: 1.13.0


The Parquet reader currently uses a hard-coded batch size limit (32k rows) when 
creating scan batches; there is no parameter nor any logic for controlling the 
amount of memory used. This enhancement will allow Drill to take an extra input 
parameter to control direct memory usage.



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

[GitHub] drill pull request #1116: DRILL-6140: Correctly list Operators in a Query Pr...

[amansinha100]

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

https://github.com/apache/drill/pull/1116#discussion_r167269862
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java
 ---
@@ -322,4 +330,16 @@ public String getOperatorsJSON() {
   public boolean isOnlyImpersonationEnabled() {
 return onlyImpersonationEnabled;
   }
+
+  //Generates operator names inferred from physical plan
+  private void generateOpMap(String plan) {
+this.physicalOperatorMap = new HashMap();
+String[] operatorLine = plan.split("\\n");
+for (String line : operatorLine) {
+  String[] lineToken = line.split("\\s+", 3);
+  String operatorPath = lineToken[0].trim().replaceFirst("-", "-xx-"); 
//Required format for lookup
--- End diff --

Can you add (as a comment) the  sample text from the physical plan output 
that this is parsing.  


---

[GitHub] drill pull request #1116: DRILL-6140: Correctly list Operators in a Query Pr...

[amansinha100]

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

https://github.com/apache/drill/pull/1116#discussion_r167286191
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -45,12 +45,25 @@
   private final String operatorName;
   private final int size;
 
-  public OperatorWrapper(int major, 
List, String>> 
opsAndHostsList) {
+  public OperatorWrapper(int major, 
List, String>> 
opsAndHostsList, Map phyOperMap) {
 Preconditions.checkArgument(opsAndHostsList.size() > 0);
 this.major = major;
 firstProfile = opsAndHostsList.get(0).getLeft().getLeft();
 operatorType = 
CoreOperatorType.valueOf(firstProfile.getOperatorType());
-operatorName = operatorType == null ? UNKNOWN_OPERATOR : 
operatorType.toString();
+//Update Name from Physical Map
+String path = new 
OperatorPathBuilder().setMajor(major).setOperator(firstProfile).build();
+//Use Plan Extracted Operator Names if available
+String extractedOpName = phyOperMap.get(path);
+String inferredOpName = operatorType == null ? UNKNOWN_OPERATOR : 
operatorType.toString();
+//Revert to inferred names for exceptional cases
+if (extractedOpName == null || inferredOpName.contains(extractedOpName)
+|| inferredOpName.endsWith("_RECEIVER") || 
inferredOpName.endsWith("_RECEIVER")) {
--- End diff --

There is an extra check for "_RECEIVER" 


---

[GitHub] drill pull request #1116: DRILL-6140: Correctly list Operators in a Query Pr...

[amansinha100]

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

https://github.com/apache/drill/pull/1116#discussion_r167288758
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -45,12 +45,25 @@
   private final String operatorName;
   private final int size;
 
-  public OperatorWrapper(int major, 
List, String>> 
opsAndHostsList) {
+  public OperatorWrapper(int major, 
List, String>> 
opsAndHostsList, Map phyOperMap) {
 Preconditions.checkArgument(opsAndHostsList.size() > 0);
 this.major = major;
 firstProfile = opsAndHostsList.get(0).getLeft().getLeft();
 operatorType = 
CoreOperatorType.valueOf(firstProfile.getOperatorType());
-operatorName = operatorType == null ? UNKNOWN_OPERATOR : 
operatorType.toString();
+//Update Name from Physical Map
+String path = new 
OperatorPathBuilder().setMajor(major).setOperator(firstProfile).build();
+//Use Plan Extracted Operator Names if available
+String extractedOpName = phyOperMap.get(path);
+String inferredOpName = operatorType == null ? UNKNOWN_OPERATOR : 
operatorType.toString();
+//Revert to inferred names for exceptional cases
--- End diff --

Can you clarify 'exception cases' .. there are 2 examples below but it is 
not immediately clear which operators would fall into this pattern.  I assume 
the main mismatch is that the physical plan has EXCHANGE operators whereas the 
profile has SENDER and RECEIVER.  


---

[GitHub] drill pull request #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a S...

[ilooner]

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

https://github.com/apache/drill/pull/1119#discussion_r167338469
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -413,6 +413,7 @@ drill.exec.options: {
 # to start at least 2 partitions then HashAgg fallbacks to this case. 
It can be
 # enabled by setting this flag to true. By default it's set to false 
such that
 # query will fail if there is not enough memory
+drill.exec.rpc.fragrunner.timeout: 3,
--- End diff --

Thanks for catching the ordering. I reduced the default to 1. 


---

[jira] [Created] (DRILL-6148) TestSortSpillWithException is sometimes failing.

[Hanumath Rao Maduri (JIRA)]

Hanumath Rao Maduri created DRILL-6148:
--

 Summary: TestSortSpillWithException is sometimes failing.
 Key: DRILL-6148
 URL: https://issues.apache.org/jira/browse/DRILL-6148
 Project: Apache Drill
  Issue Type: Bug
  Components: Tools, Build  Test
Affects Versions: 1.12.0
Reporter: Hanumath Rao Maduri
Assignee: Hanumath Rao Maduri
 Fix For: 1.12.0


TestSortSpillWithException#testSpillLeakManaged is sometimes failing. However 
for some reason this is being observed only in one of my branch. 

TestSpillLeakManaged tests for leak when an exception is thrown during the 
spilling of the rows in ExternalSort. In the test failure case it happens that 
ExternalSort is able to sort the data with the given memory and not spill at 
all. Hence the injection interruption path is not hit at all and hence no 
exception is thrown.

The test case should use drill.exec.sort.external.mem_limit to force it to use 
as less memory as possible so as to test the case.

 

 



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

[GitHub] drill pull request #1120: DILL-6148: TestSortSpillWithException is sometimes...

[HanumathRao]

GitHub user HanumathRao opened a pull request:

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

DILL-6148: TestSortSpillWithException is sometimes failing.

I have changed the test case to use the EXTERNAL_SORT_MAX_MEMORY to 
configure the sort operator to use less memory instead of relying on 
MAX_QUERY_MEMORY_PER_NODE_KEY.

This way the test case passes all the time (it is failing intermittently in 
my branch).

@paul-rogers  can you please review this change.

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

$ git pull https://github.com/HanumathRao/drill DRILL-6148

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

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


commit 1d7798848c114c3ee1e50aef8a62ac37e4f5e596
Author: Hanumath Rao Maduri 
Date:   2018-02-09T22:13:04Z

DILL-6148: TestSortSpillWithException is sometimes failing.




---

[GitHub] drill pull request #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a S...

[Ben-Zvi]

Github user Ben-Zvi commented on a diff in the pull request:

https://github.com/apache/drill/pull/1119#discussion_r167343642
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
 ---
@@ -212,7 +212,8 @@
   new OptionDefinition(ExecConstants.CPU_LOAD_AVERAGE),
   new OptionDefinition(ExecConstants.ENABLE_VECTOR_VALIDATOR),
   new OptionDefinition(ExecConstants.ENABLE_ITERATOR_VALIDATOR),
-  new OptionDefinition(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, true, false))
+  new OptionDefinition(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, true, false)),
+  new 
OptionDefinition(ExecConstants.FRAG_RUNNER_RPC_TIMEOUT_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, false, true)),
--- End diff --

Question: Should the last two parameters be instead:
   adminOnly = true
   internal = false 



---

[GitHub] drill issue #1107: DRILL-6123: Limit batch size for Merge Join based on memo...

[ppadma]

Github user ppadma commented on the issue:

https://github.com/apache/drill/pull/1107
  
@paul-rogers Thank you very much for the review. Updated the PR with review 
comments taken care of. Please take a look. 


---

[jira] [Created] (DRILL-6149) Support Query Level Options

[Timothy Farkas (JIRA)]

Timothy Farkas created DRILL-6149:
-

 Summary: Support Query Level Options
 Key: DRILL-6149
 URL: https://issues.apache.org/jira/browse/DRILL-6149
 Project: Apache Drill
  Issue Type: Bug
Reporter: Timothy Farkas
Assignee: Venkata Jyothsna Donapati


Currently the line between session options and query level options are blurred. 
Ideally a session level option can be set and persisted for the duration of a 
user's session. A query level option can be set for a query. Right now there is 
no explicit query level option. A query level option is a session option whose 
validator has a non zero ttl. We should change this to have options that belong 
to the Query Scope as defined in the OptionDefinition. The ttl should also be 
configurable by the user in their **alter set** command.



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

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[ppadma]

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

https://github.com/apache/drill/pull/1107#discussion_r167380394
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/spill/RecordBatchSizer.java
 ---
@@ -137,10 +137,10 @@ public ColumnSize(ValueVector v, String prefix) {
   case MAP:
   case UNION:
 // No standard size for Union type
-dataSize = v.getPayloadByteCount(valueCount);
+dataSize = valueCount == 0 ? 0 : v.getPayloadByteCount(valueCount);
--- End diff --

When we get empty batch, getPayloadByteCount will throw out of bounds 
exception if the underlying method tries to read the value vector for index 0 
i.e. when it is trying to read offset vector. I added the check in those 
methods and removed here. 

I still have to retain the check here for the case when we are trying to 
read the offset vector.


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[ppadma]

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

https://github.com/apache/drill/pull/1107#discussion_r167380761
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/spill/RecordBatchSizer.java
 ---
@@ -311,8 +311,8 @@ public static ColumnSize getColumn(ValueVector v, 
String prefix) {
 
   public RecordBatchSizer(RecordBatch batch) {
 this(batch,
- (batch.getSchema().getSelectionVectorMode() == 
BatchSchema.SelectionVectorMode.TWO_BYTE) ?
- batch.getSelectionVector2() : null);
+  (batch.getSchema() == null ? null : 
(batch.getSchema().getSelectionVectorMode() == 
BatchSchema.SelectionVectorMode.TWO_BYTE ?
--- End diff --

yes, we can get empty batches with empty schema  and I think it makes sense 
to add the check here instead of calling code. That way, it be transparently 
handled underneath. 


---

[GitHub] drill issue #1105: DRILL-6125: Fix possible memory leak when query is cancel...

[ilooner]

Github user ilooner commented on the issue:

https://github.com/apache/drill/pull/1105
  
@sachouche I traced through the code. The updateAggregateStats method is 
called, which then calls the getOutgoingBatches method of the code generated 
Partitioners. That method is just a simple getter. So no one is acquiring the 
same lock. But even if someone else was, the code in the close method is single 
threaded, and synchronize blocks are reentrant. 


---

[GitHub] drill pull request #1107: DRILL-6123: Limit batch size for Merge Join based ...

[ppadma]

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

https://github.com/apache/drill/pull/1107#discussion_r167380917
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatchMemoryManager.java
 ---
@@ -0,0 +1,63 @@
+/*
+ * 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.record;
+
+import org.apache.drill.exec.physical.impl.spill.RecordBatchSizer;
+import org.apache.drill.exec.vector.ValueVector;
+
+public abstract class AbstractRecordBatchMemoryManager {
+  private int outgoingRowWidth;
+  private int outputRowCount = ValueVector.MAX_ROW_COUNT;
+  protected static final int OFFSET_VECTOR_WIDTH = 4;
+  protected static final int WORST_CASE_FRAGMENTATION_FACTOR = 2;
+  protected static final int MAX_NUM_ROWS = ValueVector.MAX_ROW_COUNT;
+  protected static final int MIN_NUM_ROWS = 1;
+
+  public void update(int inputIndex) {};
+
+  public void update() {};
+
+  public int getOutputRowCount() {
+return outputRowCount;
+  }
+
+  /**
+   * Given batchSize and rowWidth, this will set output rowCount taking 
into account
+   * the min and max that is allowed.
+   */
+  public void setOutputRowCount(long batchSize, int rowWidth) {
+this.outputRowCount = Math.min(ValueVector.MAX_ROW_COUNT,
+  
Math.max(RecordBatchSizer.safeDivide(batchSize/WORST_CASE_FRAGMENTATION_FACTOR, 
rowWidth), MIN_NUM_ROWS));
--- End diff --

batchSize here is the configured outputBatchSize that we should conform to, 
not the incoming batch size. 


---

[GitHub] drill issue #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a SystemOp...

[vrozov]

Github user vrozov commented on the issue:

https://github.com/apache/drill/pull/1119
  
LGTM


---

[GitHub] drill pull request #1119: DRILL-6143: Made FragmentsRunner's rpc timeout a S...

[ilooner]

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

https://github.com/apache/drill/pull/1119#discussion_r167346417
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
 ---
@@ -212,7 +212,8 @@
   new OptionDefinition(ExecConstants.CPU_LOAD_AVERAGE),
   new OptionDefinition(ExecConstants.ENABLE_VECTOR_VALIDATOR),
   new OptionDefinition(ExecConstants.ENABLE_ITERATOR_VALIDATOR),
-  new OptionDefinition(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, true, false))
+  new OptionDefinition(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, true, false)),
+  new 
OptionDefinition(ExecConstants.FRAG_RUNNER_RPC_TIMEOUT_VALIDATOR, new 
OptionMetaData(OptionValue.AccessibleScopes.SYSTEM, false, true)),
--- End diff --

internal should be true since we want this to show up in the internal 
options table and not the standard system options table. Changing to adminOnly 
= true seems reasonable.


---

Re: Batch Sizing for Parquet Flat Reader

[Parth Chandra]

Is there a JIRA for this? Would be useful to capture the comments in the
JIRA. Note that the document itself is not comment-able as it is shared
with view-only permissions.

Some thoughts in no particular order-
1) The Page based statistical approach is likely to run into trouble with
the encoding used for Parquet fields especially RLE which drastically
changes the size of the field. So pageSize/numValues is going to be wildly
inaccurate with RLE.
2) Not sure where you were going with the predicate pushdown section and
how it pertains to your proposed batch sizing.
3) Assuming that you go with the average batch size calculation approach,
are you proposing to have a Parquet scan specific overflow implementation?
Or are you planning to leverage the ResultSet loader mechanism? If you plan
to use the latter, it will need to be enhanced to handle a bulk chunk as
opposed to a single value at a time. If not using the ResultSet loader
mechanism, why not (you would be reinventing the wheel) ?
4) Parquet page level stats are probably not reliable. You can assume page
size (compressed/uncompressed) and value count are accurate, but nothing
else.

Also note that memory allocations by Netty greater than the 16MB chunk size
are returned to the OS when the memory is free'd. Both this document and
the original document on memory fragmentation state incorrectly that such
memory is not released back to the OS. A quick thought experiment - where
does this memory go if it is not released back to the OS?



On Fri, Feb 9, 2018 at 7:12 AM, salim achouche  wrote:

> The following document
>  9RwG4h0sI81KI5ZEvJ7HzgClCUFpB5WE/edit?ts=5a793606#>
> describes
> a proposal for enforcing batch sizing constraints (count and memory) within
> the Parquet Reader (Flat Schema). Please feel free to take a look and
> provide feedback.
>
> Thanks!
>
> Regards,
> Salim
>