You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by GitBox <gi...@apache.org> on 2020/10/16 10:50:37 UTC
[GitHub] [carbondata] akkio-97 opened a new pull request #3987: [WIP ]local-dictionary
akkio-97 opened a new pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987
### Why is this PR needed?
### What changes were proposed in this PR?
### Does this PR introduce any user interface change?
- No
- Yes. (please explain the change and update document)
### Is any new testcase added?
- No
- Yes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] brijoobopanna commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
brijoobopanna commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-714658897
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715151930
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4658/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [WIP ]local-dictionary
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-712851661
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4540/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [WIP ]local-dictionary
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-709975490
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4487/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
akkio-97 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713309055
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713272161
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4554/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 removed a comment on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
akkio-97 removed a comment on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713309055
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ydvpankaj99 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
ydvpankaj99 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713228390
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [WIP ]local-dictionary
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-709975025
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2733/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715332125
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2912/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715331462
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4667/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
akkio-97 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715002753
> please rebase, compile and push.
> And I hope you have locally compiled prestodb and prestosql both.
yes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713298067
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2804/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] asfgit closed pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-714733234
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2888/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat edited a comment on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
ajantha-bhat edited a comment on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-714924009
please rebase, compile and push.
And I hope you have locally compiled prestodb and prestosql both.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715270845
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2908/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715194344
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2904/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-714924009
please rebase, compile and push
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713149620
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2788/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713310868
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715270525
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4663/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-715333548
LGTM
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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<Integer> 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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-714721854
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4644/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713427073
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4567/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713180217
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4542/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713509134
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4584/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [WIP ]local-dictionary
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-712853297
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2786/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] akkio-97 commented on a change in pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3987: [CARBONDATA-4039] Support Local dictionary for Presto complex datatypes
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3987:
URL: https://github.com/apache/carbondata/pull/3987#issuecomment-713411001
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2816/
----------------------------------------------------------------
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:
users@infra.apache.org