[GitHub] carbondata pull request #2274: [CARBONDATA-2440] default value of ENABLE_OFF...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189478224
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    +   *
    +   * @return carbon properties instance
    +   */
    +  public static CarbonProperties getInstance();
    +```
    +
    +```
    +/**
    +   * This method will be used to add a new property
    +   *
    +   * @param key
    +   * @return properties value
    --- End diff --
   
    return CarbonProperties object，not properties value


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189478551
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -32,6 +32,8 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
          fields[1] = new Field("age", DataTypes.INT);
     
          Schema schema = new Schema(fields);
    +
    +     CarbonProperties.getInstance().addProperty("enable.offheap.sort", "false");
    --- End diff --
   
    it should input CarbonProperties first.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189478703
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    --- End diff --
   
    please optimize the Scala Style: indent


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189478710
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    +   *
    +   * @return carbon properties instance
    +   */
    +  public static CarbonProperties getInstance();
    +```
    +
    +```
    +/**
    +   * This method will be used to add a new property
    --- End diff --
   
    please optimize the Scala Style: indent


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189478893
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    +   *
    +   * @return carbon properties instance
    +   */
    +  public static CarbonProperties getInstance();
    +```
    +
    +```
    +/**
    +   * This method will be used to add a new property
    +   *
    +   * @param key
    +   * @return properties value
    +   */
    +  public CarbonProperties addProperty(String key, String value);
    +```
    +
    +```
    +/**
    +   * This method will be used to get the property value. if property is not
    --- End diff --
   
    if should be If


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189479058
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    +   *
    +   * @return carbon properties instance
    +   */
    +  public static CarbonProperties getInstance();
    +```
    +
    +```
    +/**
    +   * This method will be used to add a new property
    +   *
    +   * @param key
    +   * @return properties value
    +   */
    +  public CarbonProperties addProperty(String key, String value);
    +```
    +
    +```
    +/**
    +   * This method will be used to get the property value. if property is not
    +   * present then it will return the default value.
    --- End diff --
   
    Are there need a comma before the 'then'?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189479291
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -347,6 +349,39 @@ public Schema(Field[] fields);
     public static Schema parseJson(String json);
     ```
     
    +### Class org.apache.carbondata.core.util.CarbonProperties
    +
    +```
    +/**
    +   * This method will be responsible to get the instance of CarbonProperties class
    +   *
    +   * @return carbon properties instance
    +   */
    +  public static CarbonProperties getInstance();
    +```
    +
    +```
    +/**
    +   * This method will be used to add a new property
    +   *
    +   * @param key
    +   * @return properties value
    +   */
    +  public CarbonProperties addProperty(String key, String value);
    +```
    +
    +```
    +/**
    +   * This method will be used to get the property value. if property is not
    +   * present then it will return the default value.
    +   *
    +   * @param key
    --- End diff --
   
    please optimize the Scala Style: indent


---

[qiuchenjian-2]

Github user rahulforallp commented on the issue:

    https://github.com/apache/carbondata/pull/2274
 
    @xubo245  review comments resolved .


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189519161
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -32,6 +32,8 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
          fields[1] = new Field("age", DataTypes.INT);
     
          Schema schema = new Schema(fields);
    +
    +     CarbonProperties.getInstance().addProperty("enable.offheap.sort", "false");
    --- End diff --
   
    this one not fix


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2274
 
    please import CarbonProperties before using it.


---

[qiuchenjian-2]

Github user rahulforallp commented on the issue:

    https://github.com/apache/carbondata/pull/2274
 
    @xubo245 import done


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2274
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5016/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2274
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4844/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189573020
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
        public static void main(String[] args) throws IOException, InvalidLoadOptionException {
    -     testSdkWriter();
    +     if(args[0] != null) {
    +     testSdkWriter(args[0]);
    --- End diff --
   
    args[0] should be boolean data type


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189575614
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
        public static void main(String[] args) throws IOException, InvalidLoadOptionException {
    -     testSdkWriter();
    +     if(args[0] != null) {
    +     testSdkWriter(args[0]);
    --- End diff --
   
    please optimize the Scala Style: indent


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189575627
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
        public static void main(String[] args) throws IOException, InvalidLoadOptionException {
    -     testSdkWriter();
    +     if(args[0] != null) {
    +     testSdkWriter(args[0]);
    +     } else {
    +     testSdkWriter("true");
    --- End diff --
   
    please optimize the Scala Style: indent


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189575742
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
        public static void main(String[] args) throws IOException, InvalidLoadOptionException {
    -     testSdkWriter();
    +     if(args[0] != null) {
    +     testSdkWriter(args[0]);
    --- End diff --
   
    that is already handled in CarbonProperties validation.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189575851
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
    --- End diff --
   
    "whle" is incorrect, please optimize.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2274#discussion_r189576627
 
    --- Diff: docs/sdk-writer-guide.md ---
    @@ -13,25 +13,33 @@ These SDK writer output contains just a carbondata and carbonindex files. No met
     
      import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
      import org.apache.carbondata.core.metadata.datatype.DataTypes;
    + import org.apache.carbondata.core.util.CarbonProperties;
      import org.apache.carbondata.sdk.file.CarbonWriter;
      import org.apache.carbondata.sdk.file.CarbonWriterBuilder;
      import org.apache.carbondata.sdk.file.Field;
      import org.apache.carbondata.sdk.file.Schema;
     
      public class TestSdk {
    -
    +
    + // pass true or false whle executing the main to use offheap memory or not
    --- End diff --
   
    Suggest use "Pass" instead of "pass".


---