[GitHub] [carbondata] akkio-97 opened a new pull request #3967: [WIP] Read after update command

[GitBox]

akkio-97 opened a new pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967


    ### Why is this PR needed?
    During vector filling, due to missing implementation of putAllByteArray() method in sliceStreamReader.java, implementation in parent class CarbonColumnVectorImpl was called.
    Hence on select query expected data never showed up.
    ### What changes were proposed in this PR?
   Implemented the missing method.
       
    ### 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:
[hidden email]

[GitBox]

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


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


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-703877351


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


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-704828397






----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-704882187


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


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-704885270


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


----------------------------------------------------------------
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]

akkio-97 commented on pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-704899485


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

[GitBox]

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


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


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-704967821


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


----------------------------------------------------------------
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]

kunal642 commented on pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-705514956


   @akkio-97 Can we add a test case for this?
   


----------------------------------------------------------------
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]

kunal642 commented on pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-705514956


   @akkio-97 Can we add a test case for this?
   


----------------------------------------------------------------
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]

akkio-97 commented on pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-706982107


   Test cases should be written using SDK **update** API to generate carbon files. And currently there is an issue in that when queried either from spark or presto. I have anyway checked it on the cluster by updating from spark and querying from presto. It works fine.


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-707090745


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


----------------------------------------------------------------
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 #3967:
URL: https://github.com/apache/carbondata/pull/3967#issuecomment-707092263


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


----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r503822155



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -242,4 +242,8 @@ public void putArray(int rowId, int offset, int length) {
       columnVector.putArray(counter++, offset, length);
     }
   }
+
+  public CarbonColumnVector getColumnVector() {

Review comment:
       `columnVector` is a field defined in parent class. shall we keep this getter in its parent class(`AbstractCarbonColumnarVector`) itself.




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r503822155



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -242,4 +242,8 @@ public void putArray(int rowId, int offset, int length) {
       columnVector.putArray(counter++, offset, length);
     }
   }
+
+  public CarbonColumnVector getColumnVector() {

Review comment:
       `columnVector` is a field defined in parent class. shall we keep this getColumnVector() in its parent class(`AbstractCarbonColumnarVector`) itself. Please add `@Override` annotation to it. It is overriding the method from `CarbonColumnVector` interface




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r503846075



##########
File path: integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/SliceStreamReader.java
##########
@@ -124,6 +124,17 @@ public void putByteArray(int rowId, int count, byte[] value) {
     }
   }
 
+  @Override
+  public void putAllByteArray(byte[] data, int offset, int length) {

Review comment:
       Need to add this method  in prestosql SliceStreamReader.java as well?




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r503880242



##########
File path: integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/SliceStreamReader.java
##########
@@ -124,6 +124,17 @@ public void putByteArray(int rowId, int count, byte[] value) {
     }
   }
 
+  @Override
+  public void putAllByteArray(byte[] data, int offset, int length) {
+    int[] lengths = getLengths();
+    int[] offsets = getOffsets();
+    for (int i = 0; i < lengths.length; i++) {
+      if (offsets[i] != 0) {

Review comment:
       Is this check required ? It is LV encoded byte array. Even offsets[0] wouldn't be 0 as it would point to offset of value field.




----------------------------------------------------------------
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]

akkio-97 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r504504630



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -242,4 +242,8 @@ public void putArray(int rowId, int offset, int length) {
       columnVector.putArray(counter++, offset, length);
     }
   }
+
+  public CarbonColumnVector getColumnVector() {

Review comment:
       Right, I have made the changes.




----------------------------------------------------------------
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]

akkio-97 commented on a change in pull request #3967:
URL: https://github.com/apache/carbondata/pull/3967#discussion_r504504712



##########
File path: integration/presto/src/main/prestodb/org/apache/carbondata/presto/readers/SliceStreamReader.java
##########
@@ -124,6 +124,17 @@ public void putByteArray(int rowId, int count, byte[] value) {
     }
   }
 
+  @Override
+  public void putAllByteArray(byte[] data, int offset, int length) {

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