[GitHub] carbondata pull request #2549: [CARBONDATA-2606][Complex DataType Enhancemen...

[qiuchenjian-2]

GitHub user dhatchayani opened a pull request:

    https://github.com/apache/carbondata/pull/2549

    [CARBONDATA-2606][Complex DataType Enhancements]Fix Null result if projection column have null primitive column and struct

    **Problem:**
    In case if the actual value of the primitive data type is null, by [PR#](https://github.com/apache/carbondata/pull/2489), we are moving all the null values to the end of the collected row without considering the data type.
   
    **Solution:**
    Place null in the end of output iff the null value is of complex primitive column.
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [x] Testing done
            UT added
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/dhatchayani/carbondata CARBONDATA-2606

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2549.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2549
   
----
commit 03643ce8a591689b5dec0c8eeed06d6eed67fcbe
Author: dhatchayani <dhatcha.official@...>
Date:   2018-07-24T13:27:41Z

    [CARBONDATA-2606][Complex DataType Enhancements]Fix Null result if projection column have null primitive column and struct

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7464/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6220/



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7476/



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6233/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    retest this please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5991/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7487/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6241/



---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2549#discussion_r205043434
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/collector/impl/DictionaryBasedResultCollector.java ---
    @@ -141,22 +141,52 @@ public DictionaryBasedResultCollector(BlockExecutionInfo blockExecutionInfos) {
           }
           fillMeasureData(scannedResult, row);
           if (scannedResult.complexParentIndexToQueryMap.toString().contains("StructQueryType")) {
    +        int[] isComplexColumn = new int[queryDimensions.length + queryMeasures.length];
    --- End diff --
   
    Please make it boolean[] , it would save space and will be meaningful to read.


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2549#discussion_r205048405
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/collector/impl/DictionaryBasedResultCollector.java ---
    @@ -141,22 +141,52 @@ public DictionaryBasedResultCollector(BlockExecutionInfo blockExecutionInfos) {
           }
           fillMeasureData(scannedResult, row);
           if (scannedResult.complexParentIndexToQueryMap.toString().contains("StructQueryType")) {
    +        int[] isComplexColumn = new int[queryDimensions.length + queryMeasures.length];
    +        for (ProjectionDimension dimension : queryDimensions) {
    +          if (null != dimension.getDimension().getComplexParentDimension()) {
    +            isComplexColumn[dimension.getOrdinal()] = 1;
    --- End diff --
   
    only checking parent complex dimension. so can we name it isComplexChildColumn ?


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2549#discussion_r205049034
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/collector/impl/DictionaryBasedResultCollector.java ---
    @@ -141,22 +141,52 @@ public DictionaryBasedResultCollector(BlockExecutionInfo blockExecutionInfos) {
           }
           fillMeasureData(scannedResult, row);
           if (scannedResult.complexParentIndexToQueryMap.toString().contains("StructQueryType")) {
    +        int[] isComplexColumn = new int[queryDimensions.length + queryMeasures.length];
    +        for (ProjectionDimension dimension : queryDimensions) {
    +          if (null != dimension.getDimension().getComplexParentDimension()) {
    +            isComplexColumn[dimension.getOrdinal()] = 1;
    +          }
    +        }
    +
             // If a : <b,c> and d : <e,f> are two struct and if a.b,a.c,d.e is given in the
             // projection list,then object array will contain a,null,d as result, because for a.b,
             // a will be filled and for a.c null will be placed.
             // Instead place null in the end of object array and send a,d,null as result.
    -        int count = 0;
             for (int j = 0; j < row.length; j++) {
    -          if (row[j] != null) row[count++] = row[j];
    +          if (row[j] == null && isComplexColumn[j] == 1) {
    +            shiftNullForStruct(row, isComplexColumn, j);
    --- End diff --
   
    does break; required here ? shiftNullForStruct seems like it is handling the consecutive nulls also. better to call  shiftNullForStruct by passing row and keep loop logic inside that, else coverity tool may give warnings.


---

[qiuchenjian-2]

Github user dhatchayani commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2549#discussion_r205050550
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/collector/impl/DictionaryBasedResultCollector.java ---
    @@ -141,22 +141,52 @@ public DictionaryBasedResultCollector(BlockExecutionInfo blockExecutionInfos) {
           }
           fillMeasureData(scannedResult, row);
           if (scannedResult.complexParentIndexToQueryMap.toString().contains("StructQueryType")) {
    +        int[] isComplexColumn = new int[queryDimensions.length + queryMeasures.length];
    +        for (ProjectionDimension dimension : queryDimensions) {
    +          if (null != dimension.getDimension().getComplexParentDimension()) {
    +            isComplexColumn[dimension.getOrdinal()] = 1;
    --- End diff --
   
    yes. If it has complex parent dimension it is a complex child column


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Also suggest to add one testcase with multiple null values as primitive as well as complex child. Just to cover all the code added.


---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    @dhatchayani Please rebase


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7503/



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Resolved conflicts and refactored the code in #2559.
   
    please close this PR


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2549
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6260/



---