[GitHub] carbondata pull request #2966: [WIP] test and check no sort by default

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2966
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9962/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2966
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9968/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2966
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9973/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2966
 
    Build Failed  with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9985/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2966#discussion_r241432600
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/FilterUtil.java ---
    @@ -2265,7 +2265,8 @@ public static int compareValues(byte[] filterValue, byte[] minMaxBytes,
           defaultValue = FilterUtil
               .getMaskKey(key, currentBlockDimension, segmentProperties.getSortColumnsGenerator());
         } else {
    -      defaultValue = ByteUtil.toXorBytes(key);
    +      defaultValue = FilterUtil
    +          .getMaskKey(key, currentBlockDimension, segmentProperties.getDimensionKeyGenerator());
    --- End diff --
   
    `if` and `else` has same code. remove if condition


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2966#discussion_r241433863
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/DataTypeUtil.java ---
    @@ -61,7 +61,35 @@
         }
       };
     
    -  private static final ThreadLocal<DateFormat> dateformatter = new ThreadLocal<DateFormat>() {
    +  private static ThreadLocal<String> timeStampformatterString = new ThreadLocal<String>() {
    +    @Override protected String initialValue() {
    +      return CarbonProperties.getInstance()
    +          .getProperty(CarbonCommonConstants.CARBON_TIMESTAMP_FORMAT,
    +              CarbonCommonConstants.CARBON_TIMESTAMP_DEFAULT_FORMAT);
    +    }
    +  };
    +
    +  private static void updateTimeStamp() {
    --- End diff --
   
    Can you describe why these changes related to this PR?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2966#discussion_r241436353
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala ---
    @@ -848,6 +848,19 @@ class TableNewProcessor(cm: TableModel) {
           tableSchema.getTableId,
           cm.databaseNameOp.getOrElse("default"))
         tablePropertiesMap.put("bad_record_path", badRecordsPath)
    +    if (tablePropertiesMap.get("sort_columns") != null) {
    +      val sortCol = tablePropertiesMap.get("sort_columns")
    +      if ((!sortCol.trim.isEmpty) && tablePropertiesMap.get("sort_scope") == null) {
    +        // If sort_scope is not specified, but sort_columns are present, set sort_scope as
    +        // local_sort in carbon_properties (cannot add in table properties as if user sets carbon
    +        // properties it won't be reflected as table properties is given higher priority)
    +        if (CarbonProperties.getInstance().getProperty(CarbonCommonConstants.LOAD_SORT_SCOPE) ==
    +            null) {
    +          CarbonProperties.getInstance()
    +            .addProperty(CarbonCommonConstants.LOAD_SORT_SCOPE, "LOCAL_SORT")
    --- End diff --
   
    I don't think we can add sort_scope directly to CarbonProperties, it changes for the complete system.
    If sortColumns are present then default table sort_scope should become local_sort not to complete system


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2966#discussion_r241437223
 
    --- Diff: integration/spark2/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala ---
    @@ -523,7 +523,8 @@ class AlterTableValidationTestCase extends Spark2QueryTest with BeforeAndAfterAl
         }
       }
     
    -  test("describe formatted for default sort_columns pre and post alter") {
    +  // after changing default sort_scope to no_sort, all dimensions are not selected for sorting.
    +  ignore("describe formatted for default sort_columns pre and post alter") {
    --- End diff --
   
    Instead of ignoring update the testcase by specifying the sort_columns while creating the table


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2966#discussion_r241437965
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -501,6 +502,17 @@ public CarbonLoadModel buildLoadModel(Schema carbonSchema)
         }
         // for the longstring field, change the datatype from string to varchar
         this.schema = updateSchemaFields(carbonSchema, longStringColumns);
    +    if (sortColumns != null && sortColumns.length != 0) {
    +      if (options == null || options.get("sort_scope") == null) {
    +        // If sort_columns are specified and sort_scope is not specified,
    +        // change sort scope to local_sort as now by default sort scope is no_sort.
    +        if (options == null) {
    +          options = new HashMap<>();
    +        }
    +        //TODO: add in carbon property instead of load options
    --- End diff --
   
    It should be tableproperty not carbon property


---