[GitHub] carbondata pull request #2651: [HOTFIX] Support TableProperties Map API for ...

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2651: [HOTFIX] Support TableProperties Map API for ...

qiuchenjian-2
GitHub user ajantha-bhat opened a pull request:

    https://github.com/apache/carbondata/pull/2651

    [HOTFIX] Support TableProperties Map API for SDK

    Currently SDK supports load options as map input. But table properties is not a map.
    So this PR supports a API that can take already supported table properties as map.
   
    This is will help for easy configuration for end user of SDK.
   
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed? NA. Added new interface.
     
     - [ ] Any backward compatibility impacted? NA
     
     - [ ] Document update required? yes, updated.
   
     - [ ] Testing done. Updated UT.
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. NA
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ajantha-bhat/carbondata unmanaged_table

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2651.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2651
   
----
commit 5671a9ff55bb23ce678319da342245ec527b0386
Author: ajantha-bhat <ajanthabhat@...>
Date:   2018-08-23T10:09:29Z

    Support TablePropertiesMap API for SDK

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
Github user ravipesala commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2651: [HOTFIX] Support TableProperties Map API for ...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user manishgupta88 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2651#discussion_r212582796
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -272,6 +272,56 @@ public CarbonWriterBuilder withLoadOptions(Map<String, String> options) {
         return this;
       }
     
    +  /**
    +   * To support the table properties for sdk writer
    +   *
    +   * @param options key,value pair of create table properties.
    +   * supported keys values are
    +   * a. blocksize -- [1-2048] values in MB. Default value is 1024
    +   * b. blockletsize -- values in MB. Default value is 64 MB
    +   * c. localDictionaryThreshold -- positive value, default is 10000
    +   * d. enableLocalDictionary -- true / false. Default is false
    +   * e. sortcolumns -- comma separated column. "c1,c2". Default all dimensions are sorted.
    +   *
    +   * @return updated CarbonWriterBuilder
    +   */
    +  public CarbonWriterBuilder withTableProperties(Map<String, String> options) {
    +    Objects.requireNonNull(options, "Table properties should not be null");
    +    //validate the options.
    +    if (options.size() > 5) {
    +      throw new IllegalArgumentException("Supports only 5 options now. "
    +          + "Refer method header or documentation");
    +    }
    +
    +    for (String option: options.keySet()) {
    +      if (!option.equalsIgnoreCase("blocksize") &&
    +          !option.equalsIgnoreCase("blockletsize") &&
    +          !option.equalsIgnoreCase("localDictionaryThreshold") &&
    +          !option.equalsIgnoreCase("enableLocalDictionary") &&
    +          !option.equalsIgnoreCase("sortcolumns")) {
    +        throw new IllegalArgumentException("Unsupported options. "
    --- End diff --
   
    Better to put all the allowed options in a set and then each property u can check using Set.Contains API which will be faster and a cleaner code


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2651: [HOTFIX] Support TableProperties Map API for ...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2651#discussion_r212589476
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java ---
    @@ -272,6 +272,56 @@ public CarbonWriterBuilder withLoadOptions(Map<String, String> options) {
         return this;
       }
     
    +  /**
    +   * To support the table properties for sdk writer
    +   *
    +   * @param options key,value pair of create table properties.
    +   * supported keys values are
    +   * a. blocksize -- [1-2048] values in MB. Default value is 1024
    +   * b. blockletsize -- values in MB. Default value is 64 MB
    +   * c. localDictionaryThreshold -- positive value, default is 10000
    +   * d. enableLocalDictionary -- true / false. Default is false
    +   * e. sortcolumns -- comma separated column. "c1,c2". Default all dimensions are sorted.
    +   *
    +   * @return updated CarbonWriterBuilder
    +   */
    +  public CarbonWriterBuilder withTableProperties(Map<String, String> options) {
    +    Objects.requireNonNull(options, "Table properties should not be null");
    +    //validate the options.
    +    if (options.size() > 5) {
    +      throw new IllegalArgumentException("Supports only 5 options now. "
    +          + "Refer method header or documentation");
    +    }
    +
    +    for (String option: options.keySet()) {
    +      if (!option.equalsIgnoreCase("blocksize") &&
    +          !option.equalsIgnoreCase("blockletsize") &&
    +          !option.equalsIgnoreCase("localDictionaryThreshold") &&
    +          !option.equalsIgnoreCase("enableLocalDictionary") &&
    +          !option.equalsIgnoreCase("sortcolumns")) {
    +        throw new IllegalArgumentException("Unsupported options. "
    --- End diff --
   
    ok. modified.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user manishgupta88 commented on the issue:

    https://github.com/apache/carbondata/pull/2651
 
    LGTM...can be merged once build is passed


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user ravipesala commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata issue #2651: [HOTFIX] Support TableProperties Map API for SDK

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] carbondata pull request #2651: [HOTFIX] Support TableProperties Map API for ...

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user asfgit closed the pull request at:

    https://github.com/apache/carbondata/pull/2651


---