[GitHub] carbondata pull request #2662: [CARBONDATA-2889]Add decoder based fallback m...

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

    https://github.com/apache/carbondata/pull/2662
 
    @akashrn5 Please publish the performance and memory report


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/2662
 
    @kumarvishal09 @jackylk i have updated the PR description with performance and memory report, i have published the result in mail also, please have a look


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214426930
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/EncodedBlocklet.java ---
    @@ -87,19 +91,24 @@ private void addPageMetadata(EncodedTablePage encodedTablePage) {
        * @param encodedTablePage
        * encoded table page
        */
    -  private void addEncodedMeasurePage(EncodedTablePage encodedTablePage) {
    +  private void addEncodedMeasurePage(EncodedTablePage encodedTablePage,
    +      Map<String, LocalDictionaryGenerator> localDictionaryGeneratorMap) {
         // for first page create new list
         if (null == encodedMeasureColumnPages) {
           encodedMeasureColumnPages = new ArrayList<>();
           // adding measure pages
           for (int i = 0; i < encodedTablePage.getNumMeasures(); i++) {
    -        BlockletEncodedColumnPage blockletEncodedColumnPage = new BlockletEncodedColumnPage(null);
    -        blockletEncodedColumnPage.addEncodedColumnColumnPage(encodedTablePage.getMeasure(i));
    +        BlockletEncodedColumnPage blockletEncodedColumnPage = new BlockletEncodedColumnPage(null,
    +            Boolean.parseBoolean(CarbonProperties.getInstance()
    --- End diff --
   
    @xuchuanyin i have tested and results i have published, i think we can keep it as property and make default by true, as we getting good result with respect to memory


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214504736
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/BlockletEncodedColumnPage.java ---
    @@ -86,7 +92,8 @@
        * @param encodedColumnPage
        * encoded column page
        */
    -  void addEncodedColumnColumnPage(EncodedColumnPage encodedColumnPage) {
    +  void addEncodedColumnColumnPage(EncodedColumnPage encodedColumnPage,
    --- End diff --
   
    Can u Please update the method name addEncodedColumnPage.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214504750
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/BlockletEncodedColumnPage.java ---
    @@ -105,8 +112,15 @@ void addEncodedColumnColumnPage(EncodedColumnPage encodedColumnPage) {
           LOGGER.info(
               "Local dictionary Fallback is initiated for column: " + this.columnName + " for page:"
                   + encodedColumnPageList.size());
    -      fallbackFutureQueue.add(fallbackExecutorService
    -          .submit(new FallbackColumnPageEncoder(encodedColumnPage, encodedColumnPageList.size())));
    +      if (isDecoderBasedFallBackEnabled) {
    --- End diff --
   
    Move this code to some private metod and pass encodedColumnPage and pageIndex


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214504804
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/EncodedBlocklet.java ---
    @@ -87,19 +91,24 @@ private void addPageMetadata(EncodedTablePage encodedTablePage) {
        * @param encodedTablePage
        * encoded table page
        */
    -  private void addEncodedMeasurePage(EncodedTablePage encodedTablePage) {
    +  private void addEncodedMeasurePage(EncodedTablePage encodedTablePage,
    +      Map<String, LocalDictionaryGenerator> localDictionaryGeneratorMap) {
         // for first page create new list
         if (null == encodedMeasureColumnPages) {
           encodedMeasureColumnPages = new ArrayList<>();
           // adding measure pages
           for (int i = 0; i < encodedTablePage.getNumMeasures(); i++) {
    -        BlockletEncodedColumnPage blockletEncodedColumnPage = new BlockletEncodedColumnPage(null);
    -        blockletEncodedColumnPage.addEncodedColumnColumnPage(encodedTablePage.getMeasure(i));
    +        BlockletEncodedColumnPage blockletEncodedColumnPage = new BlockletEncodedColumnPage(null,
    +            Boolean.parseBoolean(CarbonProperties.getInstance()
    --- End diff --
   
    Instead of parsing every time for each encodedPage parse once in constructor and add in private field ...for measure u can directly pass false as local dictionary will not generated for measure page


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214505217
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/BlockletEncodedColumnPage.java ---
    @@ -86,7 +92,8 @@
        * @param encodedColumnPage
        * encoded column page
        */
    -  void addEncodedColumnColumnPage(EncodedColumnPage encodedColumnPage) {
    +  void addEncodedColumnColumnPage(EncodedColumnPage encodedColumnPage,
    +      LocalDictionaryGenerator localDictionaryGenerator) {
    --- End diff --
   
    better to add local dictionary generator in constructor


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214505230
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/FallbackActualDataBasedColumnPageEncoder.java ---
    @@ -19,17 +19,17 @@
     import java.util.concurrent.Callable;
     
     import org.apache.carbondata.core.datastore.TableSpec;
    -import org.apache.carbondata.core.datastore.page.encoding.ColumnPageEncoder;
    -import org.apache.carbondata.core.datastore.page.encoding.DefaultEncodingFactory;
     import org.apache.carbondata.core.datastore.page.encoding.EncodedColumnPage;
    +import org.apache.carbondata.core.util.CarbonUtil;
     
     /**
      * Below class will be used to encode column pages for which local dictionary was generated
      * but all the pages in blocklet was not encoded with local dictionary.
      * This is required as all the pages of a column in blocklet either it will be local dictionary
      * encoded or without local dictionary encoded.
      */
    -public class FallbackColumnPageEncoder implements Callable<FallbackEncodedColumnPage> {
    +public class FallbackActualDataBasedColumnPageEncoder
    --- End diff --
   
    Change class name to ActualDataBasedFallbackEncoder


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214505238
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/FallbackDecoderBasedColumnPageEncoder.java ---
    @@ -0,0 +1,132 @@
    +/*
    + * 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.datastore.page;
    +
    +import java.nio.ByteBuffer;
    +import java.util.concurrent.Callable;
    +
    +import org.apache.carbondata.core.constants.CarbonCommonConstants;
    +import org.apache.carbondata.core.datastore.TableSpec;
    +import org.apache.carbondata.core.datastore.columnar.UnBlockIndexer;
    +import org.apache.carbondata.core.datastore.compression.CompressorFactory;
    +import org.apache.carbondata.core.datastore.page.encoding.EncodedColumnPage;
    +import org.apache.carbondata.core.keygenerator.KeyGenerator;
    +import org.apache.carbondata.core.keygenerator.factory.KeyGeneratorFactory;
    +import org.apache.carbondata.core.localdictionary.generator.LocalDictionaryGenerator;
    +import org.apache.carbondata.core.metadata.datatype.DataType;
    +import org.apache.carbondata.core.util.CarbonUtil;
    +import org.apache.carbondata.format.Encoding;
    +
    +public class FallbackDecoderBasedColumnPageEncoder implements Callable<FallbackEncodedColumnPage> {
    --- End diff --
   
    change class name to DecoderBasedFallbackEncoder


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/2662
 
    @kumarvishal09 handled comments, please review


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2662#discussion_r214545340
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/blocklet/EncodedBlocklet.java ---
    @@ -110,21 +120,24 @@ private void addEncodedMeasurePage(EncodedTablePage encodedTablePage) {
        * @param encodedTablePage
        * encoded table page
        */
    -  private void addEncodedDimensionPage(EncodedTablePage encodedTablePage) {
    +  private void addEncodedDimensionPage(EncodedTablePage encodedTablePage,
    +      Map<String, LocalDictionaryGenerator> localDictionaryGeneratorMap) {
    --- End diff --
   
    Local dictionary map instance will not change and EncodedBlocklet instance is created only once, better to pass in constructor


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---