[GitHub] carbondata pull request #2403: [CARBONDATA-2632][BloomDataMap] Fix bugs in b...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2403#discussion_r199041852
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomDataMapBuilder.java ---
    @@ -42,6 +42,8 @@
           boolean bloomCompress) throws IOException {
         super(tablePath, dataMapName, indexColumns, segment, shardName, bloomFilterSize, bloomFilterFpp,
             bloomCompress);
    +    throw new RuntimeException(
    --- End diff --
   
    Use UnsupportedOperationException


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2403
 
    @jackylk I refactored the commit based on our discussion, please check.
   
    Some tests are added to clarify the scenarios


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2403#discussion_r199325781
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomDataMapWriter.java ---
    @@ -69,6 +86,27 @@
         indexBloomFilters = new ArrayList<>(indexColumns.size());
         initDataMapFile();
         resetBloomFilters();
    +
    +    keyGenerator = segmentProperties.getDimensionKeyGenerator();
    --- End diff --
   
    Can we optimize this instead of passing the whole `SegmentProperties` into this Writer class? Please check @ravipesala


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2403#discussion_r199325844
 
    --- Diff: datamap/bloom/pom.xml ---
    @@ -23,6 +23,18 @@
           <artifactId>carbondata-core</artifactId>
           <version>${project.version}</version>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.carbondata</groupId>
    +      <artifactId>carbondata-processing</artifactId>
    +      <version>${project.version}</version>
    +    </dependency>
    +    <!--note: guava 14.0.1 is omitted during assembly.
    +    The compile scope here is for building and running test-->
    --- End diff --
   
    you have not added compiler scope


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2403#discussion_r199325848
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/converter/impl/DirectDictionaryFieldConverterImpl.java ---
    @@ -65,16 +65,22 @@ public DirectDictionaryFieldConverterImpl(DataField dataField, String nullFormat
       @Override
       public void convert(CarbonRow row, BadRecordLogHolder logHolder) {
         String value = row.getString(index);
    -    if (value == null) {
    +    row.update(convert(value, logHolder), index);
    +  }
    +
    +  @Override public Object convert(Object value, BadRecordLogHolder logHolder)
    +      throws RuntimeException {
    +    String literalValue = (String) value;
    +    if (literalValue == null) {
           logHolder.setReason(
               CarbonDataProcessorUtil.prepareFailureReason(column.getColName(), column.getDataType()));
    -      row.update(1, index);
    -    } else if (value.equals(nullFormat)) {
    -      row.update(1, index);
    +      return 1;
    --- End diff --
   
    Suggest to create a constant for null value (1)


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2403#discussion_r199377946
 
    --- Diff: datamap/bloom/pom.xml ---
    @@ -23,6 +23,18 @@
           <artifactId>carbondata-core</artifactId>
           <version>${project.version}</version>
         </dependency>
    +    <dependency>
    +      <groupId>org.apache.carbondata</groupId>
    +      <artifactId>carbondata-processing</artifactId>
    +      <version>${project.version}</version>
    +    </dependency>
    +    <!--note: guava 14.0.1 is omitted during assembly.
    +    The compile scope here is for building and running test-->
    --- End diff --
   
    Oh, this line is to be removed. It is used for guava-cache previously. will fix it


---