[GitHub] [carbondata] akkio-97 opened a new pull request #3943: [CARBONDATA-4000] Presto select query failure right after firing update query.

[GitBox]

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


    ### Why is this PR needed?
    The putObject() method implementation was missing due to which select query on **struct** complex data type after **update** queries were failing.
   
    ### What changes were proposed in this PR?
   Added implementation in ColumnarVectorWrapperDirectWithDeleteDelta. Earlier without this implementation control went to its parent class AbstractCarbonColumnarVector,java which was unsupported.
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   
   
       
   


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

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



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -58,6 +58,17 @@ public void putBoolean(int rowId, boolean value) {
     }
   }
 
+  @Override

Review comment:
       please check and handle for `ColumnarVectorWrapperDirectWithInvertedIndex`, `ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex` also




----------------------------------------------------------------
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 #3943:
URL: https://github.com/apache/carbondata/pull/3943#discussion_r491961229



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -58,6 +58,17 @@ public void putBoolean(int rowId, boolean value) {
     }
   }
 
+  @Override

Review comment:
       Done. Issue is with regards to only update query so changes may not be required in ColumnarVectorWrapperDirectWithInvertedIndex.




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

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



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex.java
##########
@@ -78,6 +80,17 @@ public void putNull(int rowId) {
     }
   }
 
+  @Override
+  public void putObject(int rowId, Object obj) {
+    if (!deletedRows.get(rowId)) {
+      if (nullBits.get(rowId)) {
+        columnVector.putNull(counter++);

Review comment:
       can you locally test one table with inverted index, I think here should not use `counter`, may be ned to use `rowId`, convert function is having `counter` already




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

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



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex.java
##########
@@ -78,6 +80,17 @@ public void putNull(int rowId) {
     }
   }
 
+  @Override
+  public void putObject(int rowId, Object obj) {
+    if (!deletedRows.get(rowId)) {
+      if (nullBits.get(rowId)) {
+        columnVector.putNull(counter++);

Review comment:
       can you locally test one table with inverted index, I think here should not use `counter`, may be need to use `rowId`, convert function is having `counter` already




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


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


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


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


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

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



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -58,6 +58,17 @@ public void putBoolean(int rowId, boolean value) {
     }
   }
 
+  @Override

Review comment:
       please check and handle for `ColumnarVectorWrapperDirectWithInvertedIndex`, `ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex` also

##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex.java
##########
@@ -78,6 +80,17 @@ public void putNull(int rowId) {
     }
   }
 
+  @Override
+  public void putObject(int rowId, Object obj) {
+    if (!deletedRows.get(rowId)) {
+      if (nullBits.get(rowId)) {
+        columnVector.putNull(counter++);

Review comment:
       can you locally test one table with inverted index, I think here should not use `counter`, may be ned to use `rowId`, convert function is having `counter` already

##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex.java
##########
@@ -78,6 +80,17 @@ public void putNull(int rowId) {
     }
   }
 
+  @Override
+  public void putObject(int rowId, Object obj) {
+    if (!deletedRows.get(rowId)) {
+      if (nullBits.get(rowId)) {
+        columnVector.putNull(counter++);

Review comment:
       can you locally test one table with inverted index, I think here should not use `counter`, may be need to use `rowId`, convert function is having `counter` already




----------------------------------------------------------------
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 #3943:
URL: https://github.com/apache/carbondata/pull/3943#discussion_r491961229



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDelta.java
##########
@@ -58,6 +58,17 @@ public void putBoolean(int rowId, boolean value) {
     }
   }
 
+  @Override

Review comment:
       Done. Issue is with regards to only update query so changes may not be required in ColumnarVectorWrapperDirectWithInvertedIndex.




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






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

ajantha-bhat commented on pull request #3943:
URL: https://github.com/apache/carbondata/pull/3943#issuecomment-696685344


   LGTM.
   Will merge once build passes.
   
   Please create jira for inverted index issue and handle in separate PR


----------------------------------------------------------------
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 #3943:
URL: https://github.com/apache/carbondata/pull/3943#discussion_r492730939



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/result/vector/impl/directread/ColumnarVectorWrapperDirectWithDeleteDeltaAndInvertedIndex.java
##########
@@ -78,6 +80,17 @@ public void putNull(int rowId) {
     }
   }
 
+  @Override
+  public void putObject(int rowId, Object obj) {
+    if (!deletedRows.get(rowId)) {
+      if (nullBits.get(rowId)) {
+        columnVector.putNull(counter++);

Review comment:
       Have raised a separate jira to address this issue - CARBONDATA-4004




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


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


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


   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]

akkio-97 removed a comment on pull request #3943:
URL: https://github.com/apache/carbondata/pull/3943#issuecomment-696750855


   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]

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


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


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


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


   > LGTM.
   > Will merge once build passes.
   >
   > Please create jira for inverted index issue and handle in separate PR
   
   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]

[GitBox]

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






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

ajantha-bhat commented on pull request #3943:
URL: https://github.com/apache/carbondata/pull/3943#issuecomment-696685344


   LGTM.
   Will merge once build passes.
   
   Please create jira for inverted index issue and handle in separate PR


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