[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

qiuchenjian-2
GitHub user jackylk opened a pull request:

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

    [CARBONDATA-2907] Support setting blocklet size in table property

    When creating table, should support setting blocklet size (property name "TABLE_BLOCKLET_SIZE"). If user does not set this table property, use system level property
   
     - [X] Any interfaces changed?
     No
     - [X] Any backward compatibility impacted?
     No
     - [X] Document update required?
    Yes, updated
     - [X] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
     two testcase added      
     - [X] 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/jackylk/incubator-carbondata fix_blockletsize

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

    https://github.com/apache/carbondata/pull/2682.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 #2682
   
----
commit ff85ebb420e67ff6400ff1e26e565f030d9d7f56
Author: Jacky Li <jacky.likun@...>
Date:   2018-09-01T16:32:29Z

    support blocklet size table property

----


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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

qiuchenjian-2
Github user CarbonDataQA commented on the issue:

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



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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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



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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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



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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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



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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214629556
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/writer/v3/CarbonFactDataWriterImplV3.java ---
    @@ -68,11 +70,14 @@
     
       public CarbonFactDataWriterImplV3(CarbonFactDataHandlerModel model) {
         super(model);
    -    blockletSizeThreshold = Long.parseLong(CarbonProperties.getInstance()
    -        .getProperty(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB,
    -            CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB_DEFAULT_VALUE))
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR;
    +    String blockletSize =
    --- End diff --
   
    I didn't see the code to verify this value's correctness


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214630184
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableSchemaBuilder.java ---
    @@ -117,7 +116,7 @@ public TableSchema build() {
           property.put(CarbonCommonConstants.TABLE_BLOCKSIZE, String.valueOf(blockSize));
         }
         if (blockletSize > 0) {
    -      property.put(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB, String.valueOf(blockletSize));
    --- End diff --
   
    Why not use the old variable directly?


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214629195
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/writer/v3/CarbonFactDataWriterImplV3.java ---
    @@ -68,11 +70,14 @@
     
       public CarbonFactDataWriterImplV3(CarbonFactDataHandlerModel model) {
         super(model);
    -    blockletSizeThreshold = Long.parseLong(CarbonProperties.getInstance()
    -        .getProperty(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB,
    -            CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB_DEFAULT_VALUE))
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR;
    +    String blockletSize =
    +        model.getTableSpec().getCarbonTable().getTableInfo().getFactTable().getTableProperties()
    +            .get(TABLE_BLOCKLET_SIZE);
    +    if (blockletSize == null) {
    --- End diff --
   
    If the user does not specify the blocklet size and the blocklet size is changed during the loading. Will it be a problem?


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214705354
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/writer/v3/CarbonFactDataWriterImplV3.java ---
    @@ -68,11 +70,14 @@
     
       public CarbonFactDataWriterImplV3(CarbonFactDataHandlerModel model) {
         super(model);
    -    blockletSizeThreshold = Long.parseLong(CarbonProperties.getInstance()
    -        .getProperty(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB,
    -            CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB_DEFAULT_VALUE))
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR;
    +    String blockletSize =
    +        model.getTableSpec().getCarbonTable().getTableInfo().getFactTable().getTableProperties()
    +            .get(TABLE_BLOCKLET_SIZE);
    +    if (blockletSize == null) {
    --- End diff --
   
    not a problem. blocklet size is get from the property for every load


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214706037
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableSchemaBuilder.java ---
    @@ -117,7 +116,7 @@ public TableSchema build() {
           property.put(CarbonCommonConstants.TABLE_BLOCKSIZE, String.valueOf(blockSize));
         }
         if (blockletSize > 0) {
    -      property.put(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB, String.valueOf(blockletSize));
    --- End diff --
   
    CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB is  "carbon.blockletgroup.size.in.mb", this string is system property, but a normal table property is without dot, like `TABLE_BLOCKSIZE = "table_blocksize";
    So I added a `TABLE_BLOCKLET_SIZE = "table_blocklet_size"`


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

    https://github.com/apache/carbondata/pull/2682#discussion_r214710189
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/writer/v3/CarbonFactDataWriterImplV3.java ---
    @@ -68,11 +70,14 @@
     
       public CarbonFactDataWriterImplV3(CarbonFactDataHandlerModel model) {
         super(model);
    -    blockletSizeThreshold = Long.parseLong(CarbonProperties.getInstance()
    -        .getProperty(CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB,
    -            CarbonV3DataFormatConstants.BLOCKLET_SIZE_IN_MB_DEFAULT_VALUE))
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR
    -        * CarbonCommonConstants.BYTE_TO_KB_CONVERSION_FACTOR;
    +    String blockletSize =
    --- End diff --
   
    I added in CarbonDDLSqlParser. And one testcase is added


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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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



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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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



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

[GitHub] carbondata issue #2682: [CARBONDATA-2907] Support setting blocklet size in t...

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

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


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

[GitHub] carbondata pull request #2682: [CARBONDATA-2907] Support setting blocklet si...

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

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


---