[GitHub] carbondata pull request #1126: [CARBONDATA-1258] CarbonData should not allow...

[qiuchenjian-2]

GitHub user mohammadshahidkhan opened a pull request:

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

    [CARBONDATA-1258] CarbonData should not allow loading Date Type value…

    …s violating the boundary condition ("0001-01-01" through "9999-12-31")
    For Date Type the value X being loaded must be X >= "0001-01-01" and X <= "9999-12-31".

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

    $ git pull https://github.com/mohammadshahidkhan/incubator-carbondata datecolumnissue

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

    https://github.com/apache/carbondata/pull/1126.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 #1126
   
----
commit 3d442e6041582332dc26eb7cc56cb5366f4f96db
Author: mohammadshahidkhan <[hidden email]>
Date:   2017-07-03T10:18:40Z

    [CARBONDATA-1258] CarbonData should not allow loading Date Type values violating the boundary condition ("0001-01-01" through "9999-12-31")

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user asfgit commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user asfgit commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/288/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2871/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1126#discussion_r127450363
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/keygenerator/directdictionary/timestamp/DateDirectDictionaryGenerator.java ---
    @@ -154,6 +162,12 @@ private int generateDirectSurrogateKeyForNonTimestampType(String memberStr) {
       }
     
       private int generateKey(long timeValue) {
    +    if (timeValue < MIN_VALUE || timeValue > MAX_VALUE) {
    +      if (LOGGER.isDebugEnabled()) {
    +        LOGGER.debug("Value for date type column is not in valid range. Value considered as null.");
    +      }
    +      return 1;
    --- End diff --
   
    why return 1?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1126#discussion_r133404888
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/keygenerator/directdictionary/timestamp/DateDirectDictionaryGenerator.java ---
    @@ -154,6 +162,12 @@ private int generateDirectSurrogateKeyForNonTimestampType(String memberStr) {
       }
     
       private int generateKey(long timeValue) {
    +    if (timeValue < MIN_VALUE || timeValue > MAX_VALUE) {
    +      if (LOGGER.isDebugEnabled()) {
    +        LOGGER.debug("Value for date type column is not in valid range. Value considered as null.");
    +      }
    +      return 1;
    --- End diff --
   
    if the value is not in defined range then it will be store as null, if bad records action is FORCE.
    For null value dictionary key is 1.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/881/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/895/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    SDV failure is random, not related to this PR


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1126#discussion_r157700973
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/keygenerator/directdictionary/timestamp/DateDirectDictionaryGenerator.java ---
    @@ -42,12 +42,37 @@
     
       private String dateFormat;
     
    +  /**
    +   * min value supported for date type column
    +   */
    +  private static final long MIN_VALUE;
    +  /**
    +   * MAx value supported for date type column
    +   */
    +  private static final long MAX_VALUE;
       /**
        * Logger instance
        */
       private static final LogService LOGGER =
           LogServiceFactory.getLogService(DateDirectDictionaryGenerator.class.getName());
     
    +  static {
    +    SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd");
    +    df.setTimeZone(TimeZone.getTimeZone("GMT"));
    +    long minValue = 0;
    +    long maxValue = 0;
    +    try {
    +      minValue = df.parse("0001-01-01").getTime();
    +      maxValue = df.parse("9999-12-31").getTime();
    +    } catch (ParseException e) {
    --- End diff --
   
    As you are defining the date format yourself and parsing it, ParseException will not be thrown. But as we need to handle this exception, you can just put a warning logger in catch block and take out the remaining logic. If we dont get any findbug for empty catch block then better to just put a comment in catch block and not do anything


---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

    https://github.com/apache/carbondata/pull/1126
 
    Correct the PR header also...remove dots from the end and put a proper message


---