[GitHub] carbondata pull request #1953: [CARBONDATA-2091][DataLoad] Support specifyin...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user jackylk commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r169989543
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -30,34 +30,32 @@
     /**
      * Bucketing information
      */
    -public class BucketingInfo implements Serializable, Writable {
    -
    +public class BucketingInfo implements ColumnRangeInfo, Serializable, Writable {
       private static final long serialVersionUID = -0L;
    -
       private List<ColumnSchema> listOfColumns;
    -
    -  private int numberOfBuckets;
    +  // number of column ranges
    --- End diff --
   
    Why is it called "column ranges"? Isn't it "value ranges"?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r169990579
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/ColumnRangeInfo.java ---
    @@ -0,0 +1,26 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.core.metadata.schema;
    +
    +/**
    + * interface for column range information. Currently we treat bucket and sort_column_range as
    + * column ranges.
    + */
    +public interface ColumnRangeInfo {
    +  int getNumOfRanges();
    +}
    --- End diff --
   
    add one new line at the end of file, otherwise it breaks code style


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r170002493
 
    --- Diff: docs/data-management-on-carbondata.md ---
    @@ -370,6 +370,17 @@ This tutorial is going to introduce all commands and data operations on CarbonDa
         ```
         NOTE: Date formats are specified by date pattern strings. The date pattern letters in CarbonData are same as in JAVA. Refer to [SimpleDateFormat](http://docs.oracle.com/javase/7/docs/api/java/text/SimpleDateFormat.html).
     
    +  - **SORT COLUMN BOUNDS:** Range bounds for sort columns.
    +
    +    ```
    +    OPTIONS('SORT_COLUMN_BOUNDS'='v11,v21,v31;v12,v22,v32;v12,v23,v33')
    --- End diff --
   
    typo, last value range is `v13,v23,v33`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r170003007
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/SortColumnRangeInfo.java ---
    @@ -0,0 +1,78 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.core.metadata.schema;
    +
    +import java.io.Serializable;
    +import java.util.Arrays;
    +
    +/**
    + * column ranges specified by sort column bounds
    + */
    +public class SortColumnRangeInfo implements ColumnRangeInfo, Serializable {
    --- End diff --
   
    For all public class, please annotate with `@InterfaceAudience`, in this PR, all newly added public class should be `@InterfaceAudience.Internal`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r170003203
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/CarbonDataLoadConfiguration.java ---
    @@ -107,6 +108,7 @@
        */
       private short writingCoresCount;
     
    +  private SortColumnRangeInfo sortColumnRangeInfo;
       public CarbonDataLoadConfiguration() {
    --- End diff --
   
    add one empty line


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r170003522
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/DataLoadProcessBuilder.java ---
    @@ -231,4 +238,71 @@ public static CarbonDataLoadConfiguration createConfiguration(CarbonLoadModel lo
         return configuration;
       }
     
    +  /**
    +   * set sort column info in configuration
    +   * @param carbonTable carbon table
    +   * @param loadModel load model
    +   * @param configuration configuration
    +   */
    +  private static void setSortColumnInfo(CarbonTable carbonTable, CarbonLoadModel loadModel,
    +      CarbonDataLoadConfiguration configuration) {
    +    List<String> sortCols = carbonTable.getSortColumns(carbonTable.getTableName());
    +    SortScopeOptions.SortScope sortScope = SortScopeOptions.getSortScope(loadModel.getSortScope());
    +    if (!SortScopeOptions.SortScope.LOCAL_SORT.equals(sortScope)
    +        || sortCols.size() == 0
    +        || StringUtils.isBlank(loadModel.getSortColumnsBoundsStr())) {
    +      if (!StringUtils.isBlank(loadModel.getSortColumnsBoundsStr())) {
    +        LOGGER.warn("sort column bounds will be ignored");
    +      }
    +
    +      configuration.setSortColumnRangeInfo(null);
    +      return;
    +    }
    +    // column index for sort columns
    +    int[] sortColIndex = new int[sortCols.size()];
    +    boolean[] isSortColNoDict = new boolean[sortCols.size()];
    +
    +    DataField[] outFields = configuration.getDataFields();
    +    int j = 0;
    +    boolean columnExist;
    +    for (String sortCol : sortCols) {
    +      columnExist = false;
    +
    +      for (int i = 0; !columnExist && i < outFields.length; i++) {
    +        if (outFields[i].getColumn().getColName().equalsIgnoreCase(sortCol)) {
    +          columnExist = true;
    +
    +          sortColIndex[j] = i;
    +          isSortColNoDict[j] = !outFields[i].hasDictionaryEncoding();
    +          j++;
    +        }
    +      }
    +
    +      if (!columnExist) {
    +        throw new RuntimeException("Field " + sortCol + " does not exist.");
    --- End diff --
   
    It  is better to use DataLoadingException


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1953#discussion_r170003740
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/partition/impl/RangePartitionerImpl.java ---
    @@ -0,0 +1,68 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.processing.loading.partition.impl;
    +
    +import java.util.Arrays;
    +import java.util.Comparator;
    +
    +import org.apache.carbondata.common.logging.LogService;
    +import org.apache.carbondata.common.logging.LogServiceFactory;
    +import org.apache.carbondata.core.datastore.row.CarbonRow;
    +import org.apache.carbondata.processing.loading.partition.Partitioner;
    +
    +public class RangePartitionerImpl implements Partitioner<CarbonRow> {
    +  private static final LogService LOGGER =
    +      LogServiceFactory.getLogService(RangePartitionerImpl.class.getName());
    +  private CarbonRow[] rangeBounds;
    +  private Comparator<CarbonRow> comparator;
    +
    +  public RangePartitionerImpl(CarbonRow[] rangeBounds, Comparator<CarbonRow> comparator) {
    +    this.rangeBounds = rangeBounds;
    +    LOGGER.info("Use range partitioner to distribute data to "
    +        + (rangeBounds.length + 1) + " ranges.");
    +    this.comparator = comparator;
    +  }
    +
    +  /**
    +   * learned from spark org.apache.spark.RangePartitioner
    +   *
    +   * @param key key
    +   * @return partitionId
    +   */
    +  @Override public int getPartition(CarbonRow key) {
    --- End diff --
   
    put all @Override to previous line


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---