[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-29 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r479657086



##
File path: 
integration/presto/src/main/prestosql/org/apache/carbondata/presto/readers/ComplexTypeStreamReader.java
##
@@ -0,0 +1,196 @@
+/*
+ * 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.carbondata.presto.readers;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+import io.prestosql.spi.block.ArrayBlock;
+import io.prestosql.spi.block.RowBlock;
+import io.prestosql.spi.type.*;
+
+import org.apache.carbondata.core.metadata.datatype.DataType;
+import org.apache.carbondata.core.metadata.datatype.DataTypes;
+import org.apache.carbondata.core.metadata.datatype.StructField;
+import org.apache.carbondata.core.scan.result.vector.CarbonColumnVector;
+import 
org.apache.carbondata.core.scan.result.vector.impl.CarbonColumnVectorImpl;
+
+import io.prestosql.spi.block.Block;
+import io.prestosql.spi.block.BlockBuilder;
+
+import org.apache.carbondata.presto.CarbonVectorBatch;
+import org.apache.carbondata.presto.ColumnarVectorWrapperDirect;
+
+/**
+ * Class to read the complex type Stream [array/struct/map]
+ */
+
+public class ComplexTypeStreamReader extends CarbonColumnVectorImpl
+implements PrestoVectorBlockBuilder {
+
+  protected int batchSize;
+
+  protected Type type;
+  protected BlockBuilder builder;
+
+  public ComplexTypeStreamReader(int batchSize, StructField field) {
+super(batchSize, field.getDataType());
+this.batchSize = batchSize;
+this.type = getType(field);
+ArrayList childrenList = new ArrayList<>();

Review comment:
   Better use Interface type, List instead of ArrayList





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-29 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r479656884



##
File path: 
core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##
@@ -187,6 +197,43 @@ public void fillVector(Object valuesToBeConverted, int 
size,
 vector.putDecimal(i, value, precision);
   }
 }
+  } else if (pageType == DataTypes.BYTE_ARRAY) {
+// complex primitive decimal dimension
+int offset = 0;
+for (int j = 0; j < size; j++) {
+  // here decimal data will be Length[4 byte], scale[1 byte], 
value[Length byte]
+  int len = ByteBuffer.wrap(data, offset, 
DataTypes.INT.getSizeInBytes()).getInt();
+  offset += DataTypes.INT.getSizeInBytes();

Review comment:
   DataTypes.INT.getSizeInBytes), It’s better to assign to some variable 
and use it inside loop, instead calling method every time 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-29 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r479656097



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -282,6 +288,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   for (int i = 0; i < size; i += DataTypes.INT.getSizeInBytes()) {
 vector.putFloat(rowId++, (max - 
ByteUtil.toIntLittleEndian(pageData, i)) / floatFactor);
   }
+} else if (pageDataType == DataTypes.LONG) {

Review comment:
   Can u pls raise a jira for above issue 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477313518



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/BlockletScannedResult.java
##
@@ -153,6 +154,9 @@
 
   private ReusableDataBuffer[] measureReusableBuffer;
 
+  // index used by dimensionReusableBuffer
+  int dimensionReusableBufferIndex = 0;

Review comment:
   private int dimensionReusableBufferIndex = 0;





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477313518



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/BlockletScannedResult.java
##
@@ -153,6 +154,9 @@
 
   private ReusableDataBuffer[] measureReusableBuffer;
 
+  // index used by dimensionReusableBuffer
+  int dimensionReusableBufferIndex = 0;

Review comment:
   change private int dimensionReusableBufferIndex;





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477313220



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/executor/impl/AbstractQueryExecutor.java
##
@@ -98,6 +98,9 @@
*/
   protected CarbonIterator queryIterator;
 
+  // Size of the ReusableDataBuffer based on the number of dimension 
projection columns
+  protected int reusableDimensionBufferSize = 0;

Review comment:
   by default Int value is 0, remove it 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477308991



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/compress/DirectCompressCodec.java
##
@@ -376,6 +456,56 @@ private void fillVector(byte[] pageData, 
CarbonColumnVector vector, DataType vec
   DecimalConverterFactory.DecimalConverter decimalConverter = 
vectorInfo.decimalConverter;
   decimalConverter.fillVector(pageData, pageSize, vectorInfo, 
nullBits, pageDataType);
 }
+  } else if (pageDataType == DataTypes.BYTE_ARRAY) {
+if (vectorDataType == DataTypes.STRING || vectorDataType == 
DataTypes.BINARY
+|| vectorDataType == DataTypes.VARCHAR) {
+  // for complex primitive string, binary, varchar type
+  int offset = 0;
+  for (int i = 0; i < pageSize; i++) {
+byte[] stringLen = new byte[DataTypes.INT.getSizeInBytes()];

Review comment:
   this byte array is not required, wrap page data inside byte buffer and 
updates  the position to get the length of the data
   By this we can avoid creating multiple bytebuffer and byte array . Pls 
handle all the places 
   
   ```
   ByteBuffer buffer = ByteBuffer.allocate(12);
   buffer.putInt(1);
   buffer.putInt(2);
   buffer.putInt(3);
   buffer.rewind();
   buffer.position(4);
   int anInt = buffer.getInt();
   ```





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477308991



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/compress/DirectCompressCodec.java
##
@@ -376,6 +456,56 @@ private void fillVector(byte[] pageData, 
CarbonColumnVector vector, DataType vec
   DecimalConverterFactory.DecimalConverter decimalConverter = 
vectorInfo.decimalConverter;
   decimalConverter.fillVector(pageData, pageSize, vectorInfo, 
nullBits, pageDataType);
 }
+  } else if (pageDataType == DataTypes.BYTE_ARRAY) {
+if (vectorDataType == DataTypes.STRING || vectorDataType == 
DataTypes.BINARY
+|| vectorDataType == DataTypes.VARCHAR) {
+  // for complex primitive string, binary, varchar type
+  int offset = 0;
+  for (int i = 0; i < pageSize; i++) {
+byte[] stringLen = new byte[DataTypes.INT.getSizeInBytes()];

Review comment:
   this byte array is not required, wrap page data inside byte buffer and 
updates  the position to get the length of the data
   By this we can avoid creating multiple bytebuffer and byte array . Pls 
handle all the places 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477308991



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/compress/DirectCompressCodec.java
##
@@ -376,6 +456,56 @@ private void fillVector(byte[] pageData, 
CarbonColumnVector vector, DataType vec
   DecimalConverterFactory.DecimalConverter decimalConverter = 
vectorInfo.decimalConverter;
   decimalConverter.fillVector(pageData, pageSize, vectorInfo, 
nullBits, pageDataType);
 }
+  } else if (pageDataType == DataTypes.BYTE_ARRAY) {
+if (vectorDataType == DataTypes.STRING || vectorDataType == 
DataTypes.BINARY
+|| vectorDataType == DataTypes.VARCHAR) {
+  // for complex primitive string, binary, varchar type
+  int offset = 0;
+  for (int i = 0; i < pageSize; i++) {
+byte[] stringLen = new byte[DataTypes.INT.getSizeInBytes()];

Review comment:
   this byte array is not required, wrap page data inside byte buffer and 
updates  the position to get the length of the data
   By this we can avoid creating multiple bytebuffer and byte array 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477292131



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,65 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  public static VectorUtil checkAndUpdateToChildVector(ColumnVectorInfo 
vectorInfo, int pageSize,
+  CarbonColumnVector vector, DataType vectorDataType) {
+VectorUtil vectorUtil = new VectorUtil(pageSize, vector, vectorDataType);
+Stack vectorStack = vectorInfo.getVectorStack();
+// check and update to child vector info
+if (vectorStack != null && vectorStack.peek() != null && 
vectorDataType.isComplexType()) {
+  if (DataTypes.isArrayType(vectorDataType)) {
+List childElementsCountForEachRow =
+((CarbonColumnVectorImpl) vector.getColumnVector())
+.getNumberOfChildrenElementsInEachRow();
+int newPageSize = 0;
+for (int childElementsCount : childElementsCountForEachRow) {
+  newPageSize += childElementsCount;
+}
+vectorUtil.setPageSize(newPageSize);
+  }
+  // child vector flow, so fill the child vector
+  CarbonColumnVector childVector = vectorStack.pop();
+  vectorUtil.setVector(childVector);
+  vectorUtil.setVectorDataType(childVector.getType());
+}
+return vectorUtil;
+  }
+
+  // Utility class to update current vector to child vector in case of complex 
type handling
+  public static class VectorUtil {
+private int pageSize;
+private CarbonColumnVector vector;
+private DataType vectorDataType;
+
+private VectorUtil(int pageSize, CarbonColumnVector vector, DataType 
vectorDataType) {
+  this.pageSize = pageSize;
+  this.vector = vector;
+  this.vectorDataType = vectorDataType;
+}
+
+public int getPageSize() {

Review comment:
   we can use org.apache.commons.lang3.tuple class for returning 3 args 
from one method , pls check if you can use here 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477289221



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,65 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  public static VectorUtil checkAndUpdateToChildVector(ColumnVectorInfo 
vectorInfo, int pageSize,
+  CarbonColumnVector vector, DataType vectorDataType) {
+VectorUtil vectorUtil = new VectorUtil(pageSize, vector, vectorDataType);
+Stack vectorStack = vectorInfo.getVectorStack();
+// check and update to child vector info
+if (vectorStack != null && vectorStack.peek() != null && 
vectorDataType.isComplexType()) {

Review comment:
   use Objects.nonNull for not null case and for null use Objects.isnull





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-26 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r477289221



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,65 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  public static VectorUtil checkAndUpdateToChildVector(ColumnVectorInfo 
vectorInfo, int pageSize,
+  CarbonColumnVector vector, DataType vectorDataType) {
+VectorUtil vectorUtil = new VectorUtil(pageSize, vector, vectorDataType);
+Stack vectorStack = vectorInfo.getVectorStack();
+// check and update to child vector info
+if (vectorStack != null && vectorStack.peek() != null && 
vectorDataType.isComplexType()) {

Review comment:
   use Objects.nonNull for not null case and for null use Objects.isnull, 
pls handle in all applicable places 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476552867



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/CarbonColumnVectorImpl.java
##
@@ -70,10 +73,14 @@
 
   private CarbonColumnVector dictionaryVector;
 
+  private List childrenVector;
+
   private LazyPageLoader lazyPage;
 
   private boolean loaded;
 
+  private ArrayList childElementsForEachRow = null;

Review comment:
   change this to List and null assignment is not required





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476550903



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/vector/CarbonColumnVector.java
##
@@ -114,4 +115,20 @@
 
   void setLazyPage(LazyPageLoader lazyPage);
 
+  // Add default implementation for interface,
+  // to avoid implementing presto required functions for spark or core module.
+  default List getChildrenVector() {
+return null;

Review comment:
   return empty list from here 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476547595



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -282,6 +288,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   for (int i = 0; i < size; i += DataTypes.INT.getSizeInBytes()) {
 vector.putFloat(rowId++, (max - 
ByteUtil.toIntLittleEndian(pageData, i)) / floatFactor);
   }
+} else if (pageDataType == DataTypes.LONG) {

Review comment:
   this means , In some scenario it's storing float data as Long [4bytes to 
8 bytes]? Can u pls check the test once





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476450315



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -255,6 +255,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   CarbonColumnVector vector = vectorInfo.vector;
   BitSet deletedRows = vectorInfo.deletedRows;
   DataType vectorDataType = vector.getType();
+  VectorUtil vectorUtil = new VectorUtil(vectorInfo, pageSize, vector, 
vectorDataType)

Review comment:
   can we handle this in caller method itself, handling in each and every 
class its not correct, pls change this in other classes too





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476450315



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -255,6 +255,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   CarbonColumnVector vector = vectorInfo.vector;
   BitSet deletedRows = vectorInfo.deletedRows;
   DataType vectorDataType = vector.getType();
+  VectorUtil vectorUtil = new VectorUtil(vectorInfo, pageSize, vector, 
vectorDataType)

Review comment:
   can we handle this in caller method itself, handling in each and class 
its not correct, pls change this in other classes too





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476450315



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -255,6 +255,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   CarbonColumnVector vector = vectorInfo.vector;
   BitSet deletedRows = vectorInfo.deletedRows;
   DataType vectorDataType = vector.getType();
+  VectorUtil vectorUtil = new VectorUtil(vectorInfo, pageSize, vector, 
vectorDataType)

Review comment:
   can we handle this in called method itself, handling in each and class 
its not correct, pls change this in other classes too





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476448290



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveDeltaFloatingCodec.java
##
@@ -282,6 +288,12 @@ public void decodeAndFillVector(byte[] pageData, 
ColumnVectorInfo vectorInfo, Bi
   for (int i = 0; i < size; i += DataTypes.INT.getSizeInBytes()) {
 vector.putFloat(rowId++, (max - 
ByteUtil.toIntLittleEndian(pageData, i)) / floatFactor);
   }
+} else if (pageDataType == DataTypes.LONG) {

Review comment:
   Why we are handling long here for float data type, for float type page 
data type cannot be long. 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476439065



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,52 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  // Utility class to update current vector to child vector in case of complex 
type handling
+  public static class VectorUtil {
+private ColumnVectorInfo vectorInfo;
+private int pageSize;
+private CarbonColumnVector vector;
+private DataType vectorDataType;
+
+public VectorUtil(ColumnVectorInfo vectorInfo, int pageSize, 
CarbonColumnVector vector,
+DataType vectorDataType) {
+  this.vectorInfo = vectorInfo;
+  this.pageSize = pageSize;
+  this.vector = vector;
+  this.vectorDataType = vectorDataType;
+}
+
+public int getPageSize() {
+  return pageSize;
+}
+
+public CarbonColumnVector getVector() {
+  return vector;
+}
+
+public DataType getVectorDataType() {
+  return vectorDataType;
+}
+
+public VectorUtil checkAndUpdateToChildVector() {

Review comment:
   change this to static and pass all the fields required to build this 
class as a params and create instance of this class here in this method, 
constructor make it private 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476433166



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,52 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  // Utility class to update current vector to child vector in case of complex 
type handling
+  public static class VectorUtil {
+private ColumnVectorInfo vectorInfo;
+private int pageSize;
+private CarbonColumnVector vector;
+private DataType vectorDataType;
+
+public VectorUtil(ColumnVectorInfo vectorInfo, int pageSize, 
CarbonColumnVector vector,
+DataType vectorDataType) {
+  this.vectorInfo = vectorInfo;
+  this.pageSize = pageSize;
+  this.vector = vector;
+  this.vectorDataType = vectorDataType;
+}
+
+public int getPageSize() {
+  return pageSize;
+}
+
+public CarbonColumnVector getVector() {
+  return vector;
+}
+
+public DataType getVectorDataType() {
+  return vectorDataType;
+}
+
+public VectorUtil checkAndUpdateToChildVector() {
+  Stack vectorStack = vectorInfo.getVectorStack();
+  if (vectorStack != null && vectorStack.peek() != null && 
vectorDataType.isComplexType()) {
+if (vectorDataType.getName().equals("ARRAY")) {

Review comment:
   can we use DataTypes.isArrayType(vectordatatype)?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [carbondata] kumarvishal09 commented on a change in pull request #3887: [CARBONDATA-3830] Support Array and Struct of all primitive type reading from presto

2020-08-25 Thread GitBox


kumarvishal09 commented on a change in pull request #3887:
URL: https://github.com/apache/carbondata/pull/3887#discussion_r476433166



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveCodec.java
##
@@ -260,4 +265,52 @@ protected String debugInfo() {
 return this.toString();
   }
 
+  // Utility class to update current vector to child vector in case of complex 
type handling
+  public static class VectorUtil {
+private ColumnVectorInfo vectorInfo;
+private int pageSize;
+private CarbonColumnVector vector;
+private DataType vectorDataType;
+
+public VectorUtil(ColumnVectorInfo vectorInfo, int pageSize, 
CarbonColumnVector vector,
+DataType vectorDataType) {
+  this.vectorInfo = vectorInfo;
+  this.pageSize = pageSize;
+  this.vector = vector;
+  this.vectorDataType = vectorDataType;
+}
+
+public int getPageSize() {
+  return pageSize;
+}
+
+public CarbonColumnVector getVector() {
+  return vector;
+}
+
+public DataType getVectorDataType() {
+  return vectorDataType;
+}
+
+public VectorUtil checkAndUpdateToChildVector() {
+  Stack vectorStack = vectorInfo.getVectorStack();
+  if (vectorStack != null && vectorStack.peek() != null && 
vectorDataType.isComplexType()) {
+if (vectorDataType.getName().equals("ARRAY")) {

Review comment:
   can we can DataTypes.isArrayType(vectordatatype)





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org