[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-23 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510813895



##
File path: 
integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/ComplexTypeStreamReader.java
##
@@ -139,7 +139,7 @@ public void putComplexObject(List offsetVector) {
   Block rowBlock = RowBlock
   .fromFieldBlocks(childBlocks.get(0).getPositionCount(), 
Optional.empty(),
   childBlocks.toArray(new Block[0]));
-  for (int position = 0; position < childBlocks.get(0).getPositionCount(); 
position++) {
+  for (int position = 0; position < offsetVector.size(); position++) {

Review comment:
   please check agian. Both prestodb and prestosql has this class
   
   
integration/presto/src/main/prestosql/org/apache/carbondata/presto/readers/ComplexTypeStreamReader.java
   
integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/ComplexTypeStreamReader.java





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-23 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510810713



##
File path: 
integration/presto/src/test/prestosql/org/apache/carbondata/presto/server/PrestoTestUtil.scala
##
@@ -114,4 +114,60 @@ object PrestoTestUtil {
   }
 }
   }
+
+  // this method depends on prestodb jdbc PrestoArray class

Review comment:
   ```suggestion
 // this method depends on prestosql jdbc PrestoArray class
   ```





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-23 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510810498



##
File path: 
integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/ComplexTypeStreamReader.java
##
@@ -139,7 +139,7 @@ public void putComplexObject(List offsetVector) {
   Block rowBlock = RowBlock
   .fromFieldBlocks(childBlocks.get(0).getPositionCount(), 
Optional.empty(),
   childBlocks.toArray(new Block[0]));
-  for (int position = 0; position < childBlocks.get(0).getPositionCount(); 
position++) {
+  for (int position = 0; position < offsetVector.size(); position++) {

Review comment:
   missed this to handle at presto sql file, I wonder how the test case 
passed for prestosql





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-21 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509009526



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/CarbonColumnVectorImpl.java
##
@@ -156,6 +159,14 @@ public void setNumberOfChildElementsForStruct(byte[] 
parentPageData, int pageSiz
 setNumberOfChildElementsInEachRow(childElementsForEachRow);
   }
 
+  public void setPositionCount(int positionCount) {
+

Review comment:
   revert this and keep an instance of slicestream check where you are 
setting the value.





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-21 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509008562



##
File path: 
integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##
@@ -36,6 +36,7 @@ import org.apache.carbondata.presto.server.{PrestoServer, 
PrestoTestUtil}
 import org.apache.carbondata.sdk.file.{CarbonWriter, Schema}
 
 
+

Review comment:
   please revert 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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-20 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509008144



##
File path: 
integration/presto/src/test/prestodb/org/apache/carbondata/presto/server/PrestoTestUtil.scala
##
@@ -114,4 +114,60 @@ object PrestoTestUtil {
   }
 }
   }
+
+  // this method depends on prestodb jdbc PrestoArray class
+  def validateArrayOfPrimitiveTypeDataWithLocalDict(actualResult: 
List[Map[String, Any]],

Review comment:
   Add it for prestosql also and compile for both prestodb and prestosql 
and run the testcase locally once with both the profile





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-20 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509004845



##
File path: 
core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/CarbonColumnVectorImpl.java
##
@@ -81,6 +82,8 @@
 
   private List childElementsForEachRow;
 
+  public DimensionRawColumnChunk rawColumnChunk;

Review comment:
   please make it private. and add getter and setter





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-20 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509004084



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/compress/DirectCompressCodec.java
##
@@ -325,6 +327,21 @@ private void fillPrimitiveType(byte[] pageData, 
CarbonColumnVector vector,
   int intSizeInBytes = DataTypes.INT.getSizeInBytes();
   int shortSizeInBytes = DataTypes.SHORT.getSizeInBytes();
   int lengthStoredInBytes;
+  // check if local dictionary is enabled for complex primitve type and 
call
+  // fillVector eventually
+  if (!vectorInfo.vectorStack.isEmpty()) {
+CarbonColumnVectorImpl tempVector =
+(CarbonColumnVectorImpl) 
(vectorInfo.vectorStack.peek().getColumnVector());
+if (tempVector.rawColumnChunk != null
+&& tempVector.rawColumnChunk.getLocalDictionary() != null) {
+  DimensionChunkStoreFactory.DimensionStoreType dimStoreType =
+  DimensionChunkStoreFactory.DimensionStoreType.LOCAL_DICT;
+  new VariableLengthDimensionColumnPage(pageData, new int[0], new 
int[0], pageSize,

Review comment:
   please move fill vector comment here, because 
VariableLengthDimensionColumnPage constructor is calling fill vector





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-20 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509002285



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/impl/LocalDictDimensionDataChunkStore.java
##
@@ -64,6 +66,14 @@ public void fillVector(int[] invertedIndex, int[] 
invertedIndexReverse, byte[] d
 int columnValueSize = dimensionDataChunkStore.getColumnValueSize();
 int rowsNum = dataLength / columnValueSize;
 CarbonColumnVector vector = vectorInfo.vector;
+if (vector.getType().isComplexType()) {
+  vector = vectorInfo.vectorStack.peek();

Review comment:
   directly call `getDirectVectorWrapperFactory`, it has 
vector.getType().isComplexType() inside only, other places like AdaptiveCodec 
uses the `getDirectVectorWrapperFactory`





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] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes

2020-10-20 Thread GitBox 


ajantha-bhat commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509001513



##
File path: 
core/src/main/java/org/apache/carbondata/core/datastore/chunk/reader/dimension/v3/DimensionChunkReaderV3.java
##
@@ -296,6 +297,12 @@ protected DimensionColumnPage 
decodeDimension(DimensionRawColumnChunk rawColumnP
 }
   }
   BitSet nullBitSet = QueryUtil.getNullBitSet(pageMetadata.presence, 
this.compressor);
+  // store rawColumnChunk for local dictionary
+  if (vectorInfo != null && !vectorInfo.vectorStack.isEmpty()) {

Review comment:
   please check if is local dictionary is present, then only store the 
rawColumnChunk





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