[GitHub] carbondata pull request #2877: [WIP] Added validation for supported format v...

[qiuchenjian-2]

GitHub user manishgupta88 opened a pull request:

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

    [WIP] Added validation for supported format version and Encoding type to throw proper exception to the user while reading a file

    **This PR contains:**
    1. Validation for columnar format version. Exception will be thrown during reading if specified format version is not valid to read.
    2. Validation for supported encoding type. Exception will be throw during reading if any of the written encoding is not supported for read in the current version.
   
     - [ ] Any interfaces changed?
   
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
           
     - [ ] 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/manishgupta88/carbondata compatability_checks

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

    https://github.com/apache/carbondata/pull/2877.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 #2877
   
----
commit b9c87b93d4cc2252a83371a658ff32f2e8dc7d1c
Author: manishgupta88 <tomanishgupta18@...>
Date:   2018-10-29T15:22:02Z

    Added validation for supported format version and Encoding type to throw proper exception to the user while reading a file

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2877
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1125/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2877
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9389/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2877
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9430/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229578078
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    +      return DIRECT_STRING;
    +    } else if (ordinal == ADAPTIVE_FLOATING.ordinal()) {
    +      return ADAPTIVE_FLOATING;
    +    } else if (ordinal == BOOL_BYTE.ordinal()) {
    +      return BOOL_BYTE;
    +    } else if (ordinal == ADAPTIVE_DELTA_FLOATING.ordinal()) {
    +      return ADAPTIVE_DELTA_FLOATING;
         } else if (ordinal == DIRECT_COMPRESS_VARCHAR.ordinal()) {
           return DIRECT_COMPRESS_VARCHAR;
         } else {
           throw new RuntimeException("create Encoding with invalid ordinal: " + ordinal);
         }
       }
    +
    +  /**
    +   * Method to validate for supported encoding types that can be read using the current version
    +   *
    +   * @param encodings
    +   * @return
    +   */
    +  public static boolean assertTrueForEncodingTypes(
    +      List<org.apache.carbondata.format.Encoding> encodings) {
    +    if (null == encodings || encodings.isEmpty()) {
    +      return true;
    +    }
    +    boolean supportedEncoding = true;
    +    for (org.apache.carbondata.format.Encoding encoder : encodings) {
    +      try {
    +        // if case is handle unsupported encoding type. An encoding not supported for read will be
    +        // added as null by thrift during deserialization
    +        if (null == encoder) {
    +          supportedEncoding = false;
    +        } else {
    +          // if given encoding name is not supported exception will be thrown. This case can come
    +          // if there is any change done in the ordinal of supported encoding
    +          Encoding.valueOf(encoder.name());
    +        }
    +      } catch (IllegalArgumentException ex) {
    +        supportedEncoding = false;
    --- End diff --
   
    Log the exception to identify which encoding name failed.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229578345
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    +      return DIRECT_STRING;
    +    } else if (ordinal == ADAPTIVE_FLOATING.ordinal()) {
    +      return ADAPTIVE_FLOATING;
    +    } else if (ordinal == BOOL_BYTE.ordinal()) {
    +      return BOOL_BYTE;
    +    } else if (ordinal == ADAPTIVE_DELTA_FLOATING.ordinal()) {
    +      return ADAPTIVE_DELTA_FLOATING;
         } else if (ordinal == DIRECT_COMPRESS_VARCHAR.ordinal()) {
           return DIRECT_COMPRESS_VARCHAR;
         } else {
           throw new RuntimeException("create Encoding with invalid ordinal: " + ordinal);
         }
       }
    +
    +  /**
    +   * Method to validate for supported encoding types that can be read using the current version
    +   *
    +   * @param encodings
    +   * @return
    +   */
    +  public static boolean assertTrueForEncodingTypes(
    --- End diff --
   
    1)This method return value is never used. So please remove the return type.
    2) Change method name validateEncodingTypes


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229578569
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java ---
    @@ -696,9 +697,6 @@ private void validateCarbonDataFileVersion() {
                 CarbonCommonConstants.CARBON_DATA_FILE_DEFAULT_VERSION);
           }
         }
    -    LOGGER.info("Carbon Current data file version: " + carbonProperties
    --- End diff --
   
    Log removal is intentional ? No need to log the current version?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229579618
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    --- End diff --
   
    DIRECT_STRING ordinal in thrift file is 10.  In this enum it is 11. Does it cause problem for old stores if any one uses this method to get encoding?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2877
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9436/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229620608
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    --- End diff --
   
    There will not be any problem as this ordinal is not used during thrift writing or any file persistence


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229627163
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java ---
    @@ -696,9 +697,6 @@ private void validateCarbonDataFileVersion() {
                 CarbonCommonConstants.CARBON_DATA_FILE_DEFAULT_VERSION);
           }
         }
    -    LOGGER.info("Carbon Current data file version: " + carbonProperties
    --- End diff --
   
    Added log


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229627207
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    +      return DIRECT_STRING;
    +    } else if (ordinal == ADAPTIVE_FLOATING.ordinal()) {
    +      return ADAPTIVE_FLOATING;
    +    } else if (ordinal == BOOL_BYTE.ordinal()) {
    +      return BOOL_BYTE;
    +    } else if (ordinal == ADAPTIVE_DELTA_FLOATING.ordinal()) {
    +      return ADAPTIVE_DELTA_FLOATING;
         } else if (ordinal == DIRECT_COMPRESS_VARCHAR.ordinal()) {
           return DIRECT_COMPRESS_VARCHAR;
         } else {
           throw new RuntimeException("create Encoding with invalid ordinal: " + ordinal);
         }
       }
    +
    +  /**
    +   * Method to validate for supported encoding types that can be read using the current version
    +   *
    +   * @param encodings
    +   * @return
    +   */
    +  public static boolean assertTrueForEncodingTypes(
    --- End diff --
   
    done


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2877#discussion_r229627245
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/encoder/Encoding.java ---
    @@ -57,10 +62,55 @@ public static Encoding valueOf(int ordinal) {
           return ADAPTIVE_DELTA_INTEGRAL;
         } else if (ordinal == RLE_INTEGRAL.ordinal()) {
           return RLE_INTEGRAL;
    +    } else if (ordinal == DIRECT_STRING.ordinal()) {
    +      return DIRECT_STRING;
    +    } else if (ordinal == ADAPTIVE_FLOATING.ordinal()) {
    +      return ADAPTIVE_FLOATING;
    +    } else if (ordinal == BOOL_BYTE.ordinal()) {
    +      return BOOL_BYTE;
    +    } else if (ordinal == ADAPTIVE_DELTA_FLOATING.ordinal()) {
    +      return ADAPTIVE_DELTA_FLOATING;
         } else if (ordinal == DIRECT_COMPRESS_VARCHAR.ordinal()) {
           return DIRECT_COMPRESS_VARCHAR;
         } else {
           throw new RuntimeException("create Encoding with invalid ordinal: " + ordinal);
         }
       }
    +
    +  /**
    +   * Method to validate for supported encoding types that can be read using the current version
    +   *
    +   * @param encodings
    +   * @return
    +   */
    +  public static boolean assertTrueForEncodingTypes(
    +      List<org.apache.carbondata.format.Encoding> encodings) {
    +    if (null == encodings || encodings.isEmpty()) {
    +      return true;
    +    }
    +    boolean supportedEncoding = true;
    +    for (org.apache.carbondata.format.Encoding encoder : encodings) {
    +      try {
    +        // if case is handle unsupported encoding type. An encoding not supported for read will be
    +        // added as null by thrift during deserialization
    +        if (null == encoder) {
    +          supportedEncoding = false;
    +        } else {
    +          // if given encoding name is not supported exception will be thrown. This case can come
    +          // if there is any change done in the ordinal of supported encoding
    +          Encoding.valueOf(encoder.name());
    +        }
    +      } catch (IllegalArgumentException ex) {
    +        supportedEncoding = false;
    --- End diff --
   
    done


---

[qiuchenjian-2]

Github user KanakaKumar commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---