[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[asfgit]

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r125083942
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -546,238 +576,400 @@ public DateTime getObject(int index) {
 public ${minor.javaType!type.javaType} getPrimitiveObject(int index) {
   return get(index);
 }
-
 
+
 public void get(int index, ${minor.class}Holder holder){
   <#if minor.class.startsWith("Decimal")>
   holder.scale = getField().getScale();
   holder.precision = getField().getPrecision();
   
 
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
 
 public void get(int index, Nullable${minor.class}Holder holder){
   holder.isSet = 1;
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
  <#-- type.width -->
- }
-
- /**
-  * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
-  * vector are accessed by position from the logical start of the vector.  
Values should be pushed
-  * onto the vector sequentially, but may be randomly accessed.
-  *   The width of each element is ${type.width} byte(s)
-  *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
-  *
-  * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
-  */
-  public final class Mutator extends BaseDataValueVector.BaseMutator {
-
-private Mutator(){};
-   /**
-* Set the element at the given index to the given value.  Note that 
widths smaller than
-* 32 bits are handled by the DrillBuf interface.
-*
-* @param index   position of the bit to set
-* @param value   value to set
-*/
+  }
+
+  /**
+   * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
+   * vector are accessed by position from the logical start of the vector. 
 Values should be pushed
+   * onto the vector sequentially, but may be randomly accessed.
+   * 
+   * The width of each element is {@link #VALUE_WIDTH} (= 
${type.width}) byte(s).
+   * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'
+   * 
+   *
+   * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
+   */
+   public final class Mutator extends BaseDataValueVector.BaseMutator {
+
+private Mutator() {};
+
+/**
+ * Set the element at the given index to the given value.  Note that 
widths smaller than
+ * 32 bits are handled by the DrillBuf interface.
+ *
+ * @param index   position of the bit to set
+ * @param value   value to set
+ */
+
   <#if (type.width > 8)>
 public void set(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
 public void setSafe(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
   while(index >= getValueCapacity()) {
 reAlloc();
   }
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
-  <#if (minor.class == "Interval")>
-public void set(int index, int months, int days, int milliseconds){
-  final int offsetIndex = index * ${type.width};
-  data.setInt(offsetIndex, months);
-  data.setInt((offsetIndex + ${minor.daysOffset}), days);
-  data.setInt((offsetIndex + ${minor.millisecondsOffset}), 
milliseconds);
+/**
+ * Set the value of a required or nullable vector. Enforces the value
+ * and size limits.
+ * @param index item to write
+ * @return true if the item was written, false if the index would
--- End diff --

Fixed here and several other places.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124958378
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -546,238 +576,400 @@ public DateTime getObject(int index) {
 public ${minor.javaType!type.javaType} getPrimitiveObject(int index) {
   return get(index);
 }
-
 
+
 public void get(int index, ${minor.class}Holder holder){
   <#if minor.class.startsWith("Decimal")>
   holder.scale = getField().getScale();
   holder.precision = getField().getPrecision();
   
 
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
 
 public void get(int index, Nullable${minor.class}Holder holder){
   holder.isSet = 1;
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
  <#-- type.width -->
- }
-
- /**
-  * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
-  * vector are accessed by position from the logical start of the vector.  
Values should be pushed
-  * onto the vector sequentially, but may be randomly accessed.
-  *   The width of each element is ${type.width} byte(s)
-  *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
-  *
-  * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
-  */
-  public final class Mutator extends BaseDataValueVector.BaseMutator {
-
-private Mutator(){};
-   /**
-* Set the element at the given index to the given value.  Note that 
widths smaller than
-* 32 bits are handled by the DrillBuf interface.
-*
-* @param index   position of the bit to set
-* @param value   value to set
-*/
+  }
+
+  /**
+   * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
+   * vector are accessed by position from the logical start of the vector. 
 Values should be pushed
+   * onto the vector sequentially, but may be randomly accessed.
+   * 
+   * The width of each element is {@link #VALUE_WIDTH} (= 
${type.width}) byte(s).
+   * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'
+   * 
+   *
+   * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
+   */
+   public final class Mutator extends BaseDataValueVector.BaseMutator {
+
+private Mutator() {};
+
+/**
+ * Set the element at the given index to the given value.  Note that 
widths smaller than
+ * 32 bits are handled by the DrillBuf interface.
+ *
+ * @param index   position of the bit to set
+ * @param value   value to set
+ */
+
   <#if (type.width > 8)>
 public void set(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
 public void setSafe(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
   while(index >= getValueCapacity()) {
 reAlloc();
   }
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
-  <#if (minor.class == "Interval")>
-public void set(int index, int months, int days, int milliseconds){
-  final int offsetIndex = index * ${type.width};
-  data.setInt(offsetIndex, months);
-  data.setInt((offsetIndex + ${minor.daysOffset}), days);
-  data.setInt((offsetIndex + ${minor.millisecondsOffset}), 
milliseconds);
+/**
+ * Set the value of a required or nullable vector. Enforces the value
+ * and size limits.
+ * @param index item to write
+ * @return true if the item was written, false if the index would
--- End diff --

What about this ?  The return type in Javadoc is not consistent with the 
return type here and a few other places in this file.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124958090
  
--- Diff: exec/vector/src/main/codegen/templates/NullableValueVectors.java 
---
@@ -674,6 +764,14 @@ public void reset(){
   setCount = 0;
   <#if type.major = "VarLen">lastSet = -1;
 }
+
+@Override
+public void exchange(ValueVector.Mutator other) {
--- End diff --

ok, thanks for the explanation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r124938564
  
--- Diff: exec/vector/src/main/codegen/templates/NullableValueVectors.java 
---
@@ -674,6 +764,14 @@ public void reset(){
   setCount = 0;
   <#if type.major = "VarLen">lastSet = -1;
 }
+
+@Override
+public void exchange(ValueVector.Mutator other) {
--- End diff --

Yes, indeed I am trusting the caller, in the same way we trust the caller 
to free a vector's buffer at the right time, to call `setSafe()` instead of 
`set()`, to write to the vector only once, and so on. In short, if someone does 
try to use `exchange()` with a vector owned by another allocator, the allocator 
is likely to complain about memory leaks.

An alternative is to always use transfer pairs, though that adds quite a 
bit of complexity for this one use case.

This method has a very specific usage: an operator writes to a vector, 
discovers that the vector overflows, and must swap buffers between two sets of 
vectors. This swapping is necessary because downstream operators depend on a 
vector instance, as does the writer, so it is necessary to swap buffers into 
and out of these fixed set of value vectors. Rather confusing, but the most 
performant solution given how vectors work today.

For this particular method, confusion is understandable. This is an 
`exchange()` on the `Mutator` class, swapping the state that nullable vector 
mutators maintain. It is called from a vector `exchange()` method earlier in 
this file. Added a comment to help future readers.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r124938101
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -546,238 +576,400 @@ public DateTime getObject(int index) {
 public ${minor.javaType!type.javaType} getPrimitiveObject(int index) {
   return get(index);
 }
-
 
+
 public void get(int index, ${minor.class}Holder holder){
   <#if minor.class.startsWith("Decimal")>
   holder.scale = getField().getScale();
   holder.precision = getField().getPrecision();
   
 
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
 
 public void get(int index, Nullable${minor.class}Holder holder){
   holder.isSet = 1;
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
  <#-- type.width -->
- }
-
- /**
-  * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
-  * vector are accessed by position from the logical start of the vector.  
Values should be pushed
-  * onto the vector sequentially, but may be randomly accessed.
-  *   The width of each element is ${type.width} byte(s)
-  *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
-  *
-  * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
-  */
-  public final class Mutator extends BaseDataValueVector.BaseMutator {
-
-private Mutator(){};
-   /**
-* Set the element at the given index to the given value.  Note that 
widths smaller than
-* 32 bits are handled by the DrillBuf interface.
-*
-* @param index   position of the bit to set
-* @param value   value to set
-*/
+  }
+
+  /**
+   * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
+   * vector are accessed by position from the logical start of the vector. 
 Values should be pushed
+   * onto the vector sequentially, but may be randomly accessed.
+   * 
+   * The width of each element is {@link #VALUE_WIDTH} (= 
${type.width}) byte(s).
+   * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'
+   * 
+   *
+   * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
+   */
+   public final class Mutator extends BaseDataValueVector.BaseMutator {
+
+private Mutator() {};
+
+/**
+ * Set the element at the given index to the given value.  Note that 
widths smaller than
+ * 32 bits are handled by the DrillBuf interface.
+ *
+ * @param index   position of the bit to set
+ * @param value   value to set
+ */
+
   <#if (type.width > 8)>
 public void set(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
 public void setSafe(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
   while(index >= getValueCapacity()) {
 reAlloc();
   }
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
-  <#if (minor.class == "Interval")>
-public void set(int index, int months, int days, int milliseconds){
-  final int offsetIndex = index * ${type.width};
-  data.setInt(offsetIndex, months);
-  data.setInt((offsetIndex + ${minor.daysOffset}), days);
-  data.setInt((offsetIndex + ${minor.millisecondsOffset}), 
milliseconds);
+/**
+ * Set the value of a required or nullable vector. Enforces the value
+ * and size limits.
+ * @param index item to write
+ * @return true if the item was written, false if the index would
+ * overfill the vector
+ */
+
+public void setScalar(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) throws 
VectorOverflowException {
--- End diff --

Actually, I copied this code from elsewhere in order to be consistent with 
prior usage... See the `set()` and `setSafe()` method declarations.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have 

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r124937740
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -31,68 +31,98 @@
 import org.apache.drill.exec.util.DecimalUtility;
 
 /**
- * ${minor.class} implements a vector of fixed width values.  Elements in 
the vector are accessed
- * by position, starting from the logical start of the vector.  Values 
should be pushed onto the
- * vector sequentially, but may be randomly accessed.
- *   The width of each element is ${type.width} byte(s)
- *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
+ * ${minor.class} implements a vector of fixed width values. Elements in 
the vector are accessed
+ * by position, starting from the logical start of the vector. Values 
should be pushed onto the
+ * vector sequentially, but may be accessed randomly.
+ * 
+ * The width of each element is {@link #VALUE_WIDTH} (= ${type.width}) 
byte<#if type.width != 1>s.
+ * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'.
+ * 
  *
  * NB: this class is automatically generated from ${.template_name} and 
ValueVectorTypes.tdd using FreeMarker.
  */
-public final class ${minor.class}Vector extends BaseDataValueVector 
implements FixedWidthVector{
+public final class ${minor.class}Vector extends BaseDataValueVector 
implements FixedWidthVector {
   private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(${minor.class}Vector.class);
 
+  /**
+   * Width of each fixed-width value.
+   */
+
+  public static final int VALUE_WIDTH = ${type.width};
+
+  /**
+   * Maximum number of values that this fixed-width vector can hold
+   * and stay below the maximum vector size limit. This is the limit
+   * enforced when the vector is used to hold values in a repeated
+   * vector.
+   */
+
+  public static final int MAX_VALUE_COUNT = MAX_BUFFER_SIZE / VALUE_WIDTH;
+
+  /**
+   * Maximum number of values that this fixed-width vector can hold
+   * and stay below the maximum vector size limit and/or stay below
+   * the maximum row count. This is the limit enforced when the
+   * vector is used to hold scalar (required or nullable) values.
+   */
+
+  public static final int MAX_SCALAR_COUNT = Math.min(MAX_ROW_COUNT, 
MAX_VALUE_COUNT);
--- End diff --

@Added comment. For me, Eclipse sometimes opens the wrong version of this 
file, preventing it from finding the declaration of symbols, so the explanation 
is not entirely unnecessary...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124710371
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -546,238 +576,400 @@ public DateTime getObject(int index) {
 public ${minor.javaType!type.javaType} getPrimitiveObject(int index) {
   return get(index);
 }
-
 
+
 public void get(int index, ${minor.class}Holder holder){
   <#if minor.class.startsWith("Decimal")>
   holder.scale = getField().getScale();
   holder.precision = getField().getPrecision();
   
 
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
 
 public void get(int index, Nullable${minor.class}Holder holder){
   holder.isSet = 1;
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
  <#-- type.width -->
- }
-
- /**
-  * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
-  * vector are accessed by position from the logical start of the vector.  
Values should be pushed
-  * onto the vector sequentially, but may be randomly accessed.
-  *   The width of each element is ${type.width} byte(s)
-  *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
-  *
-  * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
-  */
-  public final class Mutator extends BaseDataValueVector.BaseMutator {
-
-private Mutator(){};
-   /**
-* Set the element at the given index to the given value.  Note that 
widths smaller than
-* 32 bits are handled by the DrillBuf interface.
-*
-* @param index   position of the bit to set
-* @param value   value to set
-*/
+  }
+
+  /**
+   * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
+   * vector are accessed by position from the logical start of the vector. 
 Values should be pushed
+   * onto the vector sequentially, but may be randomly accessed.
+   * 
+   * The width of each element is {@link #VALUE_WIDTH} (= 
${type.width}) byte(s).
+   * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'
+   * 
+   *
+   * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
+   */
+   public final class Mutator extends BaseDataValueVector.BaseMutator {
+
+private Mutator() {};
+
+/**
+ * Set the element at the given index to the given value.  Note that 
widths smaller than
+ * 32 bits are handled by the DrillBuf interface.
+ *
+ * @param index   position of the bit to set
+ * @param value   value to set
+ */
+
   <#if (type.width > 8)>
 public void set(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
 public void setSafe(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
   while(index >= getValueCapacity()) {
 reAlloc();
   }
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
-  <#if (minor.class == "Interval")>
-public void set(int index, int months, int days, int milliseconds){
-  final int offsetIndex = index * ${type.width};
-  data.setInt(offsetIndex, months);
-  data.setInt((offsetIndex + ${minor.daysOffset}), days);
-  data.setInt((offsetIndex + ${minor.millisecondsOffset}), 
milliseconds);
+/**
+ * Set the value of a required or nullable vector. Enforces the value
+ * and size limits.
+ * @param index item to write
+ * @return true if the item was written, false if the index would
+ * overfill the vector
+ */
+
+public void setScalar(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) throws 
VectorOverflowException {
--- End diff --

Is there a more readable way of doing this ?  Although this is template 
code, putting an <#if> - <#else> macro in the function signature seems odd.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does 

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124835442
  
--- Diff: 
exec/vector/src/main/java/org/apache/drill/exec/vector/VectorUtils.java ---
@@ -0,0 +1,56 @@
+/*
+ * 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.vector;
+
+public class VectorUtils {
+
+  /**
+   * Vectors cannot be any larger than the Netty memory allocation
+   * block size.
+   */
+
+  private static final int ABSOLUTE_MAX_SIZE = 16 * 1024 * 1024;
+  private static final int ABSOLUTE_MIN_SIZE = 16 * 1024;
--- End diff --

It wasn't clear why the absolute_min_size is 16K .. that's not related to 
Netty I presume.  Couldn't it be much smaller ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124833987
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -546,238 +576,400 @@ public DateTime getObject(int index) {
 public ${minor.javaType!type.javaType} getPrimitiveObject(int index) {
   return get(index);
 }
-
 
+
 public void get(int index, ${minor.class}Holder holder){
   <#if minor.class.startsWith("Decimal")>
   holder.scale = getField().getScale();
   holder.precision = getField().getPrecision();
   
 
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
 
 public void get(int index, Nullable${minor.class}Holder holder){
   holder.isSet = 1;
-  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * ${type.width});
+  holder.value = 
data.get${(minor.javaType!type.javaType)?cap_first}(index * VALUE_WIDTH);
 }
  <#-- type.width -->
- }
-
- /**
-  * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
-  * vector are accessed by position from the logical start of the vector.  
Values should be pushed
-  * onto the vector sequentially, but may be randomly accessed.
-  *   The width of each element is ${type.width} byte(s)
-  *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
-  *
-  * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
-  */
-  public final class Mutator extends BaseDataValueVector.BaseMutator {
-
-private Mutator(){};
-   /**
-* Set the element at the given index to the given value.  Note that 
widths smaller than
-* 32 bits are handled by the DrillBuf interface.
-*
-* @param index   position of the bit to set
-* @param value   value to set
-*/
+  }
+
+  /**
+   * ${minor.class}.Mutator implements a mutable vector of fixed width 
values.  Elements in the
+   * vector are accessed by position from the logical start of the vector. 
 Values should be pushed
+   * onto the vector sequentially, but may be randomly accessed.
+   * 
+   * The width of each element is {@link #VALUE_WIDTH} (= 
${type.width}) byte(s).
+   * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'
+   * 
+   *
+   * NB: this class is automatically generated from ValueVectorTypes.tdd 
using FreeMarker.
+   */
+   public final class Mutator extends BaseDataValueVector.BaseMutator {
+
+private Mutator() {};
+
+/**
+ * Set the element at the given index to the given value.  Note that 
widths smaller than
+ * 32 bits are handled by the DrillBuf interface.
+ *
+ * @param index   position of the bit to set
+ * @param value   value to set
+ */
+
   <#if (type.width > 8)>
 public void set(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
 public void setSafe(int index, <#if (type.width > 
4)>${minor.javaType!type.javaType}<#else>int value) {
   while(index >= getValueCapacity()) {
 reAlloc();
   }
-  data.setBytes(index * ${type.width}, value, 0, ${type.width});
+  data.setBytes(index * VALUE_WIDTH, value, 0, VALUE_WIDTH);
 }
 
-  <#if (minor.class == "Interval")>
-public void set(int index, int months, int days, int milliseconds){
-  final int offsetIndex = index * ${type.width};
-  data.setInt(offsetIndex, months);
-  data.setInt((offsetIndex + ${minor.daysOffset}), days);
-  data.setInt((offsetIndex + ${minor.millisecondsOffset}), 
milliseconds);
+/**
+ * Set the value of a required or nullable vector. Enforces the value
+ * and size limits.
+ * @param index item to write
+ * @return true if the item was written, false if the index would
--- End diff --

Method returns a void but comment says the method returns a boolean..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124706419
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -31,68 +31,98 @@
 import org.apache.drill.exec.util.DecimalUtility;
 
 /**
- * ${minor.class} implements a vector of fixed width values.  Elements in 
the vector are accessed
- * by position, starting from the logical start of the vector.  Values 
should be pushed onto the
- * vector sequentially, but may be randomly accessed.
- *   The width of each element is ${type.width} byte(s)
- *   The equivalent Java primitive is '${minor.javaType!type.javaType}'
+ * ${minor.class} implements a vector of fixed width values. Elements in 
the vector are accessed
+ * by position, starting from the logical start of the vector. Values 
should be pushed onto the
+ * vector sequentially, but may be accessed randomly.
+ * 
+ * The width of each element is {@link #VALUE_WIDTH} (= ${type.width}) 
byte<#if type.width != 1>s.
+ * The equivalent Java primitive is 
'${minor.javaType!type.javaType}'.
+ * 
  *
  * NB: this class is automatically generated from ${.template_name} and 
ValueVectorTypes.tdd using FreeMarker.
  */
-public final class ${minor.class}Vector extends BaseDataValueVector 
implements FixedWidthVector{
+public final class ${minor.class}Vector extends BaseDataValueVector 
implements FixedWidthVector {
   private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(${minor.class}Vector.class);
 
+  /**
+   * Width of each fixed-width value.
+   */
+
+  public static final int VALUE_WIDTH = ${type.width};
+
+  /**
+   * Maximum number of values that this fixed-width vector can hold
+   * and stay below the maximum vector size limit. This is the limit
+   * enforced when the vector is used to hold values in a repeated
+   * vector.
+   */
+
+  public static final int MAX_VALUE_COUNT = MAX_BUFFER_SIZE / VALUE_WIDTH;
+
+  /**
+   * Maximum number of values that this fixed-width vector can hold
+   * and stay below the maximum vector size limit and/or stay below
+   * the maximum row count. This is the limit enforced when the
+   * vector is used to hold scalar (required or nullable) values.
+   */
+
+  public static final int MAX_SCALAR_COUNT = Math.min(MAX_ROW_COUNT, 
MAX_VALUE_COUNT);
--- End diff --

Where is the constant MAX_ROW_COUNT defined ?   (later I saw it defined in 
the ValueVector interface...perhaps add a comment here)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[amansinha100]

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

https://github.com/apache/drill/pull/840#discussion_r124813604
  
--- Diff: exec/vector/src/main/codegen/templates/NullableValueVectors.java 
---
@@ -674,6 +764,14 @@ public void reset(){
   setCount = 0;
   <#if type.major = "VarLen">lastSet = -1;
 }
+
+@Override
+public void exchange(ValueVector.Mutator other) {
--- End diff --

How does swapping the value count exchange the buffer ?  The name 
'exchange' indicates more than what this method is doing.  The JIRA description 
says the 'exchange is for vectors within a single operator'  but I am not sure 
how that is enforced considering the ValueVector.Mutator that is passed in 
could belong to any ValueVector.  So, essentially we are trusting the caller to 
use it only for specific cases. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r123134097
  
--- Diff: exec/vector/src/main/codegen/templates/VariableLengthVectors.java 
---
@@ -548,6 +567,23 @@ public void setSafe(int index, ByteBuffer bytes, int 
start, int length) {
   }
 }
 
+public void setScalar(int index, DrillBuf bytes, int start, int 
length) throws VectorOverflowException {
+  assert index >= 0;
+
+  if (index >= MAX_ROW_COUNT) {
+throw new VectorOverflowException();
+  }
+  int currentOffset = offsetVector.getAccessor().get(index);
+  final int newSize = currentOffset + length;
+  if (newSize > MAX_BUFFER_SIZE) {
+throw new VectorOverflowException();
+  }
+  while (! data.setBytesBounded(currentOffset, bytes, start, length)) {
--- End diff --

Sorry, where? Looks OK to me...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r123131374
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -806,10 +998,32 @@ public void generateTestDataAlt(int size) {
 }
 
<#-- type.width -->
+/**
+ * Backfill missing offsets from the given last written position to the
+ * given current write position. Used by the "new" size-safe column
+ * writers to allow skipping values. The set() and 
setSafe()
+ * do not fill empties. See DRILL-5529 and DRILL-.
+ * @param lastWrite the position of the last valid write: the offset
+ * to be copied forward
+ * @param index the current write position filling occurs up to,
+ * but not including, this position
+ */
+
+public void fillEmptiesBounded(int lastWrite, int index)
+throws VectorOverflowException {
+  for (int i = lastWrite; i < index; i++) {
+<#if type.width <= 8>
--- End diff --

Fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r123129202
  
--- Diff: 
exec/memory/base/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java ---
@@ -174,6 +175,40 @@ public ByteBuf setDouble(int index, double value) {
 return this;
   }
 
+  // Clone of the super class checkIndex, but this version returns a 
boolean rather
+  // than throwing an exception.
+
+  protected boolean hasCapacity(int index, int fieldLength) {
+if (fieldLength < 0) {
+throw new IllegalArgumentException("length: " + fieldLength + " 
(expected: >= 0)");
+}
+return (! (index < 0 || index > capacity() - fieldLength));
+  }
+
+  // Clone of the super class setBytes(), but with bounds checking done as 
a boolean,
+  // not assertion.
+
+  public boolean setBytesBounded(int index, byte[] src, int srcIndex, int 
length) {
+if (! hasCapacity(index, length)) {
+  return false;
+}
+if (length != 0) {
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

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

https://github.com/apache/drill/pull/840#discussion_r123129149
  
--- Diff: 
exec/memory/base/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java ---
@@ -174,6 +175,40 @@ public ByteBuf setDouble(int index, double value) {
 return this;
   }
 
+  // Clone of the super class checkIndex, but this version returns a 
boolean rather
+  // than throwing an exception.
+
+  protected boolean hasCapacity(int index, int fieldLength) {
+if (fieldLength < 0) {
--- End diff --

Fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[bitblender]

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

https://github.com/apache/drill/pull/840#discussion_r122533121
  
--- Diff: exec/vector/src/main/codegen/templates/VariableLengthVectors.java 
---
@@ -548,6 +567,23 @@ public void setSafe(int index, ByteBuffer bytes, int 
start, int length) {
   }
 }
 
+public void setScalar(int index, DrillBuf bytes, int start, int 
length) throws VectorOverflowException {
+  assert index >= 0;
+
+  if (index >= MAX_ROW_COUNT) {
+throw new VectorOverflowException();
+  }
+  int currentOffset = offsetVector.getAccessor().get(index);
+  final int newSize = currentOffset + length;
+  if (newSize > MAX_BUFFER_SIZE) {
+throw new VectorOverflowException();
+  }
+  while (! data.setBytesBounded(currentOffset, bytes, start, length)) {
--- End diff --

indentation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[bitblender]

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

https://github.com/apache/drill/pull/840#discussion_r122541355
  
--- Diff: 
exec/memory/base/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java ---
@@ -174,6 +175,40 @@ public ByteBuf setDouble(int index, double value) {
 return this;
   }
 
+  // Clone of the super class checkIndex, but this version returns a 
boolean rather
+  // than throwing an exception.
+
+  protected boolean hasCapacity(int index, int fieldLength) {
+if (fieldLength < 0) {
+throw new IllegalArgumentException("length: " + fieldLength + " 
(expected: >= 0)");
+}
+return (! (index < 0 || index > capacity() - fieldLength));
+  }
+
+  // Clone of the super class setBytes(), but with bounds checking done as 
a boolean,
+  // not assertion.
+
+  public boolean setBytesBounded(int index, byte[] src, int srcIndex, int 
length) {
+if (! hasCapacity(index, length)) {
+  return false;
+}
+if (length != 0) {
--- End diff --

Remove length check


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[bitblender]

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

https://github.com/apache/drill/pull/840#discussion_r122526465
  
--- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
@@ -806,10 +998,32 @@ public void generateTestDataAlt(int size) {
 }
 
<#-- type.width -->
+/**
+ * Backfill missing offsets from the given last written position to the
+ * given current write position. Used by the "new" size-safe column
+ * writers to allow skipping values. The set() and 
setSafe()
+ * do not fill empties. See DRILL-5529 and DRILL-.
+ * @param lastWrite the position of the last valid write: the offset
+ * to be copied forward
+ * @param index the current write position filling occurs up to,
+ * but not including, this position
+ */
+
+public void fillEmptiesBounded(int lastWrite, int index)
+throws VectorOverflowException {
+  for (int i = lastWrite; i < index; i++) {
+<#if type.width <= 8>
--- End diff --

You can avoid an extra op in the loop by adjusting the bounds of the 
induction variable. The compiler's  induction variable analysis might 
automatically figure this one out.

public void fillEmptiesBounded(int lastWrite, int index)
throws VectorOverflowException {
  for (int i = lastWrite + 1; i <= index; i++) {
setSafe(i, (int) 0);<-- 
one less addition in the loop and less register pressure
  }
}


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[bitblender]

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

https://github.com/apache/drill/pull/840#discussion_r118806486
  
--- Diff: 
exec/memory/base/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java ---
@@ -174,6 +175,40 @@ public ByteBuf setDouble(int index, double value) {
 return this;
   }
 
+  // Clone of the super class checkIndex, but this version returns a 
boolean rather
+  // than throwing an exception.
+
+  protected boolean hasCapacity(int index, int fieldLength) {
+if (fieldLength < 0) {
--- End diff --

change this to an assertion as per our discussion?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill pull request #840: DRILL-5517: Size-aware set methods in value vectors

[paul-rogers]

GitHub user paul-rogers opened a pull request:

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

DRILL-5517: Size-aware set methods in value vectors

Please see DRILL-5517 for an explanation.

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

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

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

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


commit 6d9d8f5e93c7ad0fd45242a3aba43334c9385239
Author: Paul Rogers 
Date:   2017-05-16T20:20:32Z

DRILL-5517: Size-aware set methods in value vectors

Please see DRILL-5517 for an explanation.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---