[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510819971
##
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:
done
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510819729
##
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:
It had passed earlier because this is just refactoring. I have anyway
added changes in other 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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r510812324
##
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:
We don't have that file in prestoDB 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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509152696
##
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:
We are calling that right below after creating dictionaryVector.
Also on line 69 - vector is intialized as sliceStreamReader before we create
dictionaryVector. Otherwise dictionaryVector will be null, which will lead to
NPE. So I think it is better if we place it above only.
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509158326
##
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:
done
##
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:
done
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509157828
##
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:
done
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509156660
##
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:
SliceStreamReader belongs to presto module. I will have to add
dependency in order to do that. So I have moved this as a default
implementation to CarbonColumnVector.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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509156660
##
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:
Moved this as a default implementation to CarbonColumnVector.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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509152696
##
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:
We are calling that right below after creating dictionaryVector.
on line 69 - vector is intialized as sliceStreamReader before we create
dictionaryVector. Otherwise dictionaryVector will be null, which will lead to
NPE
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509152696
##
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:
We are calling that right below after creating dictionary block.
on line 69 - vector is intialized as sliceStreamReader before we create
dictionaryBlock. Otherwise dictionaryBlock will be null, which will lead to NPE
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509152696
##
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:
We are calling that right below after creating dictionary block.
on line 69 - vector is intialized as sliceStreamReader before we create
dictionaryVector. Otherwise dictionaryVector will be null, which will lead to
NPE
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509152696
##
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:
We are calling that right below after creating dictionary block.
on line 69 - vector should be sliceStreamReader before we create
dictionaryBlock. Otherwise dictionaryBlock will be null.
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509151296
##
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:
done
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] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
akkio-97 commented on a change in pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#discussion_r509150967
##
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:
done
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
