[GitHub] carbondata pull request #3014: [WIP] Added load level SORT_SCOPE

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3014
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10423/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3014
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10448/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/3014
 
    LGTM


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3014
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10453/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user NamanRastogi commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3014
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10457/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/3014#discussion_r245879143
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/SessionParams.java ---
    @@ -229,6 +229,12 @@ private boolean validateKeyValue(String key, String value) throws InvalidConfigu
               if (!isValid) {
                 throw new InvalidConfigurationException("Invalid value " + value + " for key " + key);
               }
    +        } else if (key.startsWith(CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE)) {
    +          isValid = CarbonUtil.isValidSortOption(value);
    +          if (!isValid) {
    +            throw new InvalidConfigurationException("The sort scope " + key
    +                + " can have only either BATCH_SORT or LOCAL_SORT or NO_SORT.");
    --- End diff --
   
    Sort scope can be global as well. Add in exception message


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/3014#discussion_r245884441
 
    --- Diff: integration/spark2/src/test/scala/org/apache/spark/carbondata/commands/SetCommandTestCase.scala ---
    @@ -128,6 +128,34 @@ class SetCommandTestCase extends Spark2QueryTest with BeforeAndAfterAll{
             sql(s"set ${CarbonLoadOptionConstants.CARBON_OPTIONS_SINGLE_PASS}"))
         }
       }
    +
    +  test(s"test set ${ CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE } for valid options") {
    +    checkAnswer(
    +      sql(s"set ${ CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE }=no_sort"),
    --- End diff --
   
    As the check property is being validated to have dbname and tablename in CarbonHiveCommands.scala therefore this test case should have failed.
   
    There is a problem in https://github.com/apache/carbondata/pull/3014/files#diff-eeaa616740e33846839055324b58aa4bR87 as all the properties with @CarbonProperty annotations are being added blindly.
   
    Please fix this.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/3014#discussion_r245895212
 
    --- Diff: integration/spark2/src/test/scala/org/apache/spark/carbondata/commands/SetCommandTestCase.scala ---
    @@ -128,6 +128,34 @@ class SetCommandTestCase extends Spark2QueryTest with BeforeAndAfterAll{
             sql(s"set ${CarbonLoadOptionConstants.CARBON_OPTIONS_SINGLE_PASS}"))
         }
       }
    +
    +  test(s"test set ${ CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE } for valid options") {
    +    checkAnswer(
    +      sql(s"set ${ CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE }=no_sort"),
    --- End diff --
   
    Done.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/3014#discussion_r245896341
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/SessionParams.java ---
    @@ -229,6 +229,12 @@ private boolean validateKeyValue(String key, String value) throws InvalidConfigu
               if (!isValid) {
                 throw new InvalidConfigurationException("Invalid value " + value + " for key " + key);
               }
    +        } else if (key.startsWith(CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE)) {
    +          isValid = CarbonUtil.isValidSortOption(value);
    +          if (!isValid) {
    +            throw new InvalidConfigurationException("The sort scope " + key
    +                + " can have only either BATCH_SORT or LOCAL_SORT or NO_SORT.");
    --- End diff --
   
    Done.


---