[GitHub] [carbondata] Indhumathi27 opened a new pull request #3874: [CARBONDATA-3931]Fix Secondary index with index column as DateType giving wrong results

[GitBox]

Indhumathi27 opened a new pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874


    ### Why is this PR needed?
    On data load to SI with date type, dictionary values is loaded from factToIndexDictColumnMapping instead of getting from wrapper.
   
    ### What changes were proposed in this PR?
   Get dictionary Keys from wrapper and convert it to Int and load to SI
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - 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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-665829286






----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-666227308


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1816/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-666227308


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1816/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-666227657


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3556/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-666227657


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3556/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

QiangCai commented on a change in pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#discussion_r466781511



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/query/SecondaryIndexQueryResultProcessor.java
##########
@@ -249,10 +249,17 @@ private void processResult(List<CarbonIterator<RowBatch>> detailQueryResultItera
   private Object[] prepareRowObjectForSorting(Object[] row) {
     ByteArrayWrapper wrapper = (ByteArrayWrapper) row[0];
     // ByteBuffer[] noDictionaryBuffer = new ByteBuffer[noDictionaryCount];
-
     List<CarbonDimension> dimensions = segmentProperties.getDimensions();
     Object[] preparedRow = new Object[dimensions.size() + measureCount];
 
+    // get dictionary values for date type
+    byte[] dictionaryKey = wrapper.getDictionaryKey();
+    int[] keyArray = ByteUtil.convertBytesToIntArray(dictionaryKey);
+    Object[] dictionaryValues = new Object[dimensionColumnCount + measureCount];
+    for (int i = 0; i < keyArray.length; i++) {
+      dictionaryValues[i] = keyArray[i];

Review comment:
       why do this copy




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#discussion_r467745874



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/query/SecondaryIndexQueryResultProcessor.java
##########
@@ -249,10 +249,17 @@ private void processResult(List<CarbonIterator<RowBatch>> detailQueryResultItera
   private Object[] prepareRowObjectForSorting(Object[] row) {
     ByteArrayWrapper wrapper = (ByteArrayWrapper) row[0];
     // ByteBuffer[] noDictionaryBuffer = new ByteBuffer[noDictionaryCount];
-
     List<CarbonDimension> dimensions = segmentProperties.getDimensions();
     Object[] preparedRow = new Object[dimensions.size() + measureCount];
 
+    // get dictionary values for date type
+    byte[] dictionaryKey = wrapper.getDictionaryKey();
+    int[] keyArray = ByteUtil.convertBytesToIntArray(dictionaryKey);
+    Object[] dictionaryValues = new Object[dimensionColumnCount + measureCount];
+    for (int i = 0; i < keyArray.length; i++) {
+      dictionaryValues[i] = keyArray[i];

Review comment:
       removed




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-671279079


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3675/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-671282761


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1936/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

QiangCai commented on a change in pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#discussion_r468361486



##########
File path: integration/spark/src/main/java/org/apache/spark/sql/secondaryindex/query/SecondaryIndexQueryResultProcessor.java
##########
@@ -259,7 +262,7 @@ private void processResult(List<CarbonIterator<RowBatch>> detailQueryResultItera
       CarbonDimension dims = dimensions.get(i);
       if (dims.hasEncoding(Encoding.DICTIONARY)) {
         // dictionary
-        preparedRow[i] = factToIndexDictColumnMapping[dictionaryIndex++];
+        preparedRow[i] = dictionaryValues[dictionaryIndex++];

Review comment:
       how about adding a method which named ByteArrayWrapper.getDictionaryKeyByIndex(int index)?
   




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-671851196


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3695/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-671852762


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1956/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

QiangCai commented on pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874#issuecomment-673319957


   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:
[hidden email]

[GitBox]

asfgit closed pull request #3874:
URL: https://github.com/apache/carbondata/pull/3874


   


----------------------------------------------------------------
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:
[hidden email]