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 ---- --- |
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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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/ --- |
In reply to this post by 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. --- |
In reply to this post by 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 --- |
In reply to this post by 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? --- |
In reply to this post by 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? --- |
In reply to this post by 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/ --- |
In reply to this post by 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 --- |
In reply to this post by 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 --- |
In reply to this post by 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 --- |
In reply to this post by 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 --- |
In reply to this post by qiuchenjian-2
Github user KanakaKumar commented on the issue:
https://github.com/apache/carbondata/pull/2877 LGTM --- |
In reply to this post by 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/ --- |
Free forum by Nabble | Edit this page |