[GitHub] carbondata pull request #1707: [CARBONDATA-1839] [DataLoad] Fix bugs and opt...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1124/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1132/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2565/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2356/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1141/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158900586
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java ---
    @@ -315,6 +319,49 @@ public boolean delete() {
         return new DataInputStream(new BufferedInputStream(stream));
       }
     
    +  @Override public DataInputStream getDataInputStream(String path, FileFactory.FileType fileType,
    --- End diff --
   
    It already has a method in this class which chooses codec depends on the extension, Better use that instead of duplicating.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158900792
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java ---
    @@ -331,6 +378,26 @@ public boolean delete() {
         return fs.create(pt, true, bufferSize, fs.getDefaultReplication(pt), blockSize);
       }
     
    +  @Override public DataOutputStream getDataOutputStream(String path, FileFactory.FileType fileType,
    --- End diff --
   
    It already has a method in this class , change that method to use codec as per the file extension


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158901253
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/holder/UnsafeSortTempFileChunkHolder.java ---
    @@ -296,11 +200,12 @@ private void fillDataForPrefetch() {
        * @throws CarbonSortKeyAndGroupByException
        */
       private Object[] getRowFromStream() throws CarbonSortKeyAndGroupByException {
    -    Object[] row = new Object[dimensionCount + measureCount];
    +    Object[] row = new Object[sortParameters.getDimColCount()
    +        + sortParameters.getComplexDimColCount() + sortParameters.getMeasureColCount()];
    --- End diff --
   
    I think there is difference with changed code compare to old code. Please double check the changes. Complex dimensions may not work


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158901262
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/holder/UnsafeSortTempFileChunkHolder.java ---
    @@ -312,7 +217,7 @@ private void fillDataForPrefetch() {
           }
     
           // write complex dimensions here.
    -      for (; dimCount < dimensionCount; dimCount++) {
    +      for (; dimCount < sortParameters.getComplexDimColCount(); dimCount++) {
    --- End diff --
   
    I think there is difference with changed code compare to old code. Please double check the changes. Complex dimensions may not work


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158901473
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/holder/UnsafeSortTempFileChunkHolder.java ---
    @@ -296,11 +200,12 @@ private void fillDataForPrefetch() {
        * @throws CarbonSortKeyAndGroupByException
        */
       private Object[] getRowFromStream() throws CarbonSortKeyAndGroupByException {
    -    Object[] row = new Object[dimensionCount + measureCount];
    +    Object[] row = new Object[sortParameters.getDimColCount()
    +        + sortParameters.getComplexDimColCount() + sortParameters.getMeasureColCount()];
    --- End diff --
   
    I think better keep the old of keeping variables at class level instead of calling method for each row.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158902533
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java ---
    @@ -315,6 +319,49 @@ public boolean delete() {
         return new DataInputStream(new BufferedInputStream(stream));
       }
     
    +  @Override public DataInputStream getDataInputStream(String path, FileFactory.FileType fileType,
    --- End diff --
   
    Fine, I'll reduce the duplicate code.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2386/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Failed with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1170/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1707
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2393/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158927577
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/holder/UnsafeSortTempFileChunkHolder.java ---
    @@ -296,11 +200,12 @@ private void fillDataForPrefetch() {
        * @throws CarbonSortKeyAndGroupByException
        */
       private Object[] getRowFromStream() throws CarbonSortKeyAndGroupByException {
    -    Object[] row = new Object[dimensionCount + measureCount];
    +    Object[] row = new Object[sortParameters.getDimColCount()
    +        + sortParameters.getComplexDimColCount() + sortParameters.getMeasureColCount()];
    --- End diff --
   
    Fine~ I also find the same issues in other files and will fix it at the same time.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1707#discussion_r158927598
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/holder/UnsafeSortTempFileChunkHolder.java ---
    @@ -312,7 +217,7 @@ private void fillDataForPrefetch() {
           }
     
           // write complex dimensions here.
    -      for (; dimCount < dimensionCount; dimCount++) {
    +      for (; dimCount < sortParameters.getComplexDimColCount(); dimCount++) {
    --- End diff --
   
    yeah, I made a mistake when applying the changes to this PR.


---