[GitHub] carbondata pull request #2692: [CARBONDATA-2879] support sort scope for sdk

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    @jackylk I feel it is better to add sortscope to carbondata file. In future if we want to do compaction it will be easy to do merge sort for batch sorted and local sorted files. For no sort we need to sort and do merge sort or just appending it.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/257/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/8329/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/259/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215609784
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/model/LoadOption.java ---
    @@ -59,7 +59,8 @@
         optionsFinal.put("fileheader", Maps.getOrDefault(options, "fileheader", ""));
         optionsFinal.put("commentchar", Maps.getOrDefault(options, "commentchar", "#"));
         optionsFinal.put("columndict", Maps.getOrDefault(options, "columndict", null));
    -
    +    optionsFinal.put("sort_scope",
    +        Maps.getOrDefault(options, "sort_scope", CarbonCommonConstants.LOAD_SORT_SCOPE_DEFAULT));
    --- End diff --
   
    Please check in the CrabonSession Load scenario we are restricting it or not. It is not supposed to use per load . It should be only for SDK


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215609931
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/model/LoadOption.java ---
    @@ -183,7 +184,6 @@
         }
     
         optionsFinal.put("single_pass", String.valueOf(singlePass));
    -    optionsFinal.put("sort_scope", "local_sort");
    --- End diff --
   
    Check for SDK flow


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215612715
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -311,15 +326,15 @@ public CarbonWriterBuilder withTableProperties(Map<String, String> options) {
         }
     
         for (Map.Entry<String, String> entry : options.entrySet()) {
    -      if (entry.getKey().equalsIgnoreCase("equalsIgnoreCase")) {
    +      if (entry.getKey().equalsIgnoreCase("blocksize")) {
             this.withBlockSize(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("blockletsize")) {
             this.withBlockletSize(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("localDictionaryThreshold")) {
             this.localDictionaryThreshold(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("enableLocalDictionary")) {
             this.enableLocalDictionary((entry.getValue().equalsIgnoreCase("true")));
    -      } else {
    +      } else if (entry.getKey().equalsIgnoreCase("sortcolumns")) {
    --- End diff --
   
    Please make sure all property names are the same as create table properties


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215628606
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/model/LoadOption.java ---
    @@ -59,7 +59,8 @@
         optionsFinal.put("fileheader", Maps.getOrDefault(options, "fileheader", ""));
         optionsFinal.put("commentchar", Maps.getOrDefault(options, "commentchar", "#"));
         optionsFinal.put("columndict", Maps.getOrDefault(options, "columndict", null));
    -
    +    optionsFinal.put("sort_scope",
    +        Maps.getOrDefault(options, "sort_scope", CarbonCommonConstants.LOAD_SORT_SCOPE_DEFAULT));
    --- End diff --
   
    yes, it should not be set here.
   
    now moved to
    CarbonLoadModelBuilder.build() , so it will set only for SDK and spark-file-format


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215628732
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/model/LoadOption.java ---
    @@ -183,7 +184,6 @@
         }
     
         optionsFinal.put("single_pass", String.valueOf(singlePass));
    -    optionsFinal.put("sort_scope", "local_sort");
    --- End diff --
   
    same fix as above. reverted here.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215634003
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -311,15 +326,15 @@ public CarbonWriterBuilder withTableProperties(Map<String, String> options) {
         }
     
         for (Map.Entry<String, String> entry : options.entrySet()) {
    -      if (entry.getKey().equalsIgnoreCase("equalsIgnoreCase")) {
    +      if (entry.getKey().equalsIgnoreCase("blocksize")) {
             this.withBlockSize(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("blockletsize")) {
             this.withBlockletSize(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("localDictionaryThreshold")) {
             this.localDictionaryThreshold(Integer.parseInt(entry.getValue()));
           } else if (entry.getKey().equalsIgnoreCase("enableLocalDictionary")) {
             this.enableLocalDictionary((entry.getValue().equalsIgnoreCase("true")));
    -      } else {
    +      } else if (entry.getKey().equalsIgnoreCase("sortcolumns")) {
    --- End diff --
   
    yes. changed it to same as table properties syntax


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    @ravipesala : All comments are handled. please check.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/127/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/295/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8365/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/128/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/296/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2692
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8366/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2692#discussion_r215859284
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/model/CarbonLoadModel.java ---
    @@ -62,6 +62,7 @@
       /* Number of thread in which sdk writer is used */
       private short sdkUserCores;
     
    +
    --- End diff --
   
    remove unnecessary changes from file


---