[GitHub] carbondata pull request #2715: [CARBONDATA-2930] Support customize column co...

[qiuchenjian-2]

GitHub user xuchuanyin opened a pull request:

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

    [CARBONDATA-2930] Support customize column compressor

    Support customize column compressor so that user can add their own
    implementation of compressor. For customize compressor, user can
    directly use its full class name while creating table or setting it to
    system env.
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [x] Any interfaces changed?
     `No`
     - [x] Any backward compatibility impacted?
     `NO`
     - [x] Document update required?
    `Yes, will update it later`
     - [x] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
    `Tests added`
            - How it is tested? Please attach test report.
    `Tested in local machine`
            - Is it a performance related change? Please attach the performance test report.
    `NA`
            - Any additional information to help reviewers in testing this change.
    `NA`
           
     - [x] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


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

    $ git pull https://github.com/xuchuanyin/carbondata 0912_customize_compressor

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

    https://github.com/apache/carbondata/pull/2715.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 #2715
   
----
commit 6e37844eeda53fd934fd23370e41cbbdf4ab29ea
Author: xuchuanyin <xuchuanyin@...>
Date:   2018-09-12T12:30:17Z

    Support customize column compressor
   
    Support customize column compressor so that user can add their own
    implementation of compressor. For customize compressor, user can
    directly use its full class name while creating table or setting it to
    system env.

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/261/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8500/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2715#discussion_r217682375
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/compression/CompressorFactory.java ---
    @@ -62,15 +67,54 @@ public Compressor getCompressor() {
       }
     
       private CompressorFactory() {
    -    for (SupportedCompressor supportedCompressor : SupportedCompressor.values()) {
    -      compressors.put(supportedCompressor.getName(), supportedCompressor);
    +    for (NativeSupportedCompressor nativeSupportedCompressor : NativeSupportedCompressor.values()) {
    +      allSupportedCompressors.put(nativeSupportedCompressor.getName(),
    +          nativeSupportedCompressor.getCompressor());
         }
       }
     
       public static CompressorFactory getInstance() {
         return COMPRESSOR_FACTORY;
       }
     
    +  /**
    +   * register the compressor using reflection.
    +   * If the class name of the compressor has already been registered before, it will return false;
    +   * If the reflection fails to work or the compressor name has problem, it will throw
    +   * RunTimeException; If it is registered successfully, it will return true.
    +   *
    +   * @param compressorClassName full class name of the compressor
    +   * @return true if register successfully, false if failed.
    +   */
    +  private Compressor registerColumnCompressor(String compressorClassName) {
    +    if (allSupportedCompressors.containsKey(compressorClassName)) {
    +      return allSupportedCompressors.get(compressorClassName);
    +    }
    +
    +    Class clazz;
    +    try {
    +      clazz = Class.forName(compressorClassName);
    +      Object instance = clazz.newInstance();
    +      if (instance instanceof Compressor) {
    +        if (!((Compressor) instance).getName().equals(compressorClassName)) {
    +          throw new RuntimeException(String.format("For not carbondata native supported compressor,"
    +              + " the result of method getName() should be the full class name. Expected '%s',"
    +              + " found '%s'", compressorClassName, ((Compressor) instance).getName()));
    +        }
    +        allSupportedCompressors.put(compressorClassName, (Compressor) instance);
    --- End diff --
   
    Please add a info log for new compression registered.



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    @xuchuanyin It is better if the compressor is name taken from tableproperties.


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    @ravipesala yeah, we do have a test "test create table with customize compressor" for this 'User can specify the customize compressor while creating table'.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2715#discussion_r217955241
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/compression/CompressorFactory.java ---
    @@ -62,15 +67,54 @@ public Compressor getCompressor() {
       }
     
       private CompressorFactory() {
    -    for (SupportedCompressor supportedCompressor : SupportedCompressor.values()) {
    -      compressors.put(supportedCompressor.getName(), supportedCompressor);
    +    for (NativeSupportedCompressor nativeSupportedCompressor : NativeSupportedCompressor.values()) {
    +      allSupportedCompressors.put(nativeSupportedCompressor.getName(),
    +          nativeSupportedCompressor.getCompressor());
         }
       }
     
       public static CompressorFactory getInstance() {
         return COMPRESSOR_FACTORY;
       }
     
    +  /**
    +   * register the compressor using reflection.
    +   * If the class name of the compressor has already been registered before, it will return false;
    +   * If the reflection fails to work or the compressor name has problem, it will throw
    +   * RunTimeException; If it is registered successfully, it will return true.
    +   *
    +   * @param compressorClassName full class name of the compressor
    +   * @return true if register successfully, false if failed.
    +   */
    +  private Compressor registerColumnCompressor(String compressorClassName) {
    +    if (allSupportedCompressors.containsKey(compressorClassName)) {
    +      return allSupportedCompressors.get(compressorClassName);
    +    }
    +
    +    Class clazz;
    +    try {
    +      clazz = Class.forName(compressorClassName);
    +      Object instance = clazz.newInstance();
    +      if (instance instanceof Compressor) {
    +        if (!((Compressor) instance).getName().equals(compressorClassName)) {
    +          throw new RuntimeException(String.format("For not carbondata native supported compressor,"
    +              + " the result of method getName() should be the full class name. Expected '%s',"
    +              + " found '%s'", compressorClassName, ((Compressor) instance).getName()));
    +        }
    +        allSupportedCompressors.put(compressorClassName, (Compressor) instance);
    --- End diff --
   
    OK~


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    all review comments are resolved


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/303/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8548/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/310/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8556/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2715#discussion_r219747797
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/dataload/TestLoadDataWithCompression.scala ---
    @@ -42,6 +44,112 @@ case class Rcd(booleanField: Boolean, shortField: Short, intField: Int, bigintFi
         dateField: String, charField: String, floatField: Float, stringDictField: String,
         stringSortField: String, stringLocalDictField: String, longStringField: String)
     
    +/**
    + * This compressor actually will not compress or decompress anything.
    + * It is used for test case of specifying customized compressor.
    + */
    +class CustomizeCompressor extends Compressor {
    +  override def getName: String = "org.apache.carbondata.integration.spark.testsuite.dataload.CustomizeCompressor"
    --- End diff --
   
    I think it is better if we take shortname from compressor and store only shortname in carbondata file. And also user can use only short name in tableproperties while creating table. It should be like how Spark's fileformat interfaces uses the shortName and resolve the interfaces using the java service.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2715#discussion_r219786560
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/dataload/TestLoadDataWithCompression.scala ---
    @@ -42,6 +44,112 @@ case class Rcd(booleanField: Boolean, shortField: Short, intField: Int, bigintFi
         dateField: String, charField: String, floatField: Float, stringDictField: String,
         stringSortField: String, stringLocalDictField: String, longStringField: String)
     
    +/**
    + * This compressor actually will not compress or decompress anything.
    + * It is used for test case of specifying customized compressor.
    + */
    +class CustomizeCompressor extends Compressor {
    +  override def getName: String = "org.apache.carbondata.integration.spark.testsuite.dataload.CustomizeCompressor"
    --- End diff --
   
    yeah, I also though about this earlier, but the problem is that we need to store the relationship between 'shortName' and 'className', which means that user has to register the compressor explicitly OR we need a property (or even a file) to manage that relationship.
    In current PR's implementation, there is no need to register. It's convenient for usage.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2715
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8776/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---