[GitHub] [carbondata] Zhangshunyu opened a new pull request #3644: Fix NPE of sort merger in range sort cases

[GitBox]

Zhangshunyu opened a new pull request #3644: Fix NPE of sort merger in range sort cases
URL: https://github.com/apache/carbondata/pull/3644
 
 
    ### Why is this PR needed?
    Fix NPE of sort merger in range sort cases.
   in ParallelReadMergeSorterWithColumnRangeImpl, finalMerger.startFinalMerge will check whether
   filesToMerge.size() == 0, if true it wil return without createRecordHolderQueue and as a result it it null, and finally the code will call finalMerger.hasNext() to checlk whether thr iterator has next, but the recordHolderHeapLocal is null, it will throw NPE.
    ### What changes were proposed in this PR?
   Add null check, of recordHolderHeapLocal is null, it means not created and need merged files == 0
       
    ### 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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592875951
 
 
   Build Failed  with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/535/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592886373
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2235/
   

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592912132
 
 
   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]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592913136
 
 
   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]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#discussion_r386008911
 
 

 ##########
 File path: processing/src/main/java/org/apache/carbondata/processing/sort/sortdata/SingleThreadFinalSortFilesMerger.java
 ##########
 @@ -306,7 +306,7 @@ private IntermediateSortTempRow getSortedRecordFromFile() throws CarbonDataWrite
    * @return more element is present
    */
   public boolean hasNext() {
-    return this.recordHolderHeapLocal.size() > 0;
+    return this.recordHolderHeapLocal != null && this.recordHolderHeapLocal.size() > 0;
 
 Review comment:
   good. can you please add a test for this scenario ?

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592915258
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/540/
   

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


With regards,
Apache Git Services

[GitBox]

Zhangshunyu commented on a change in pull request #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#discussion_r386013017
 
 

 ##########
 File path: processing/src/main/java/org/apache/carbondata/processing/sort/sortdata/SingleThreadFinalSortFilesMerger.java
 ##########
 @@ -306,7 +306,7 @@ private IntermediateSortTempRow getSortedRecordFromFile() throws CarbonDataWrite
    * @return more element is present
    */
   public boolean hasNext() {
-    return this.recordHolderHeapLocal.size() > 0;
+    return this.recordHolderHeapLocal != null && this.recordHolderHeapLocal.size() > 0;
 
 Review comment:
   @ajantha-bhat after bucket table optimized and working fine can this NPE problem be exposed, so the test cases added with bucket table feature test cases in another pr based on bucket working. By the way, from code anylyse we alse can find this parameter might be null without npe checking.

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#issuecomment-592921020
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2240/
   

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#discussion_r386032476
 
 

 ##########
 File path: processing/src/main/java/org/apache/carbondata/processing/sort/sortdata/SingleThreadFinalSortFilesMerger.java
 ##########
 @@ -306,7 +306,7 @@ private IntermediateSortTempRow getSortedRecordFromFile() throws CarbonDataWrite
    * @return more element is present
    */
   public boolean hasNext() {
-    return this.recordHolderHeapLocal.size() > 0;
+    return this.recordHolderHeapLocal != null && this.recordHolderHeapLocal.size() > 0;
 
 Review comment:
   Then I suggest to keep this change in bucketing optimization PR itself as it has a testcase to cover this scenario.

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


With regards,
Apache Git Services

[GitBox]

Zhangshunyu commented on a change in pull request #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644#discussion_r386034697
 
 

 ##########
 File path: processing/src/main/java/org/apache/carbondata/processing/sort/sortdata/SingleThreadFinalSortFilesMerger.java
 ##########
 @@ -306,7 +306,7 @@ private IntermediateSortTempRow getSortedRecordFromFile() throws CarbonDataWrite
    * @return more element is present
    */
   public boolean hasNext() {
-    return this.recordHolderHeapLocal.size() > 0;
+    return this.recordHolderHeapLocal != null && this.recordHolderHeapLocal.size() > 0;
 
 Review comment:
   @ajantha-bhat ok, will close this pr and add the change into bucket table feature.

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


With regards,
Apache Git Services

[GitBox]

Zhangshunyu closed pull request #3644: [CARBONDATA-3727] Fix NPE of sort merger
URL: https://github.com/apache/carbondata/pull/3644
 
 
   

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


With regards,
Apache Git Services