[GitHub] carbondata pull request #1666: table status timezone problem fix

[qiuchenjian-2]

GitHub user mohammadshahidkhan opened a pull request:

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

    table status timezone problem fix

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] 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/mohammadshahidkhan/incubator-carbondata tablestatus_timezone_fix

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

    https://github.com/apache/carbondata/pull/1666.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 #1666
   
----
commit dbfd0db3453ca4ee83869ecfd4c83b78373e22c0
Author: mohammadshahidkhan <[hidden email]>
Date:   2017-12-15T09:04:18Z

    table status timezone problem fix

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on the issue:

    https://github.com/apache/carbondata/pull/1666
 
    This PR is ok, no code change after first push only pushed the commit message change
    @ravipesala Please review


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on the issue:

    https://github.com/apache/carbondata/pull/1666
 
    Failed Test in SDV build is unrelated, its randomly failing with other PR #1659,
    but retested got paased.


---

[qiuchenjian-2]

Github user mohammadshahidkhan commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1666#discussion_r157356451
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/LoadMetadataDetails.java ---
    @@ -241,26 +248,28 @@ private long convertTimeStampToLong(String factTimeStamp) {
        * @return
        */
       public Long getTimeStamp(String loadStartTime) {
    -    Date dateToStr = null;
         try {
    -      dateToStr = parser.parse(loadStartTime);
    -      return dateToStr.getTime() * 1000;
    -    } catch (ParseException e) {
    -      LOGGER.error("Cannot convert" + loadStartTime + " to Time/Long type value" + e.getMessage());
    -      return null;
    +      // for new loads the factTimeStamp will be long string
    --- End diff --
   
    In future for maintenance it is not easy to understand what new load is, can you add more background information for better maintenance.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1666#discussion_r157408394
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/LoadMetadataDetails.java ---
    @@ -241,26 +248,28 @@ private long convertTimeStampToLong(String factTimeStamp) {
        * @return
        */
       public Long getTimeStamp(String loadStartTime) {
    -    Date dateToStr = null;
         try {
    -      dateToStr = parser.parse(loadStartTime);
    -      return dateToStr.getTime() * 1000;
    -    } catch (ParseException e) {
    -      LOGGER.error("Cannot convert" + loadStartTime + " to Time/Long type value" + e.getMessage());
    -      return null;
    +      // for new loads the factTimeStamp will be long string
    --- End diff --
   
    Fixed Added the detailed comment at class level


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1666#discussion_r157420112
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/LoadMetadataDetails.java ---
    @@ -211,25 +234,32 @@ public long getLoadStartTimeAsLong() {
        * @return
        */
       private long convertTimeStampToLong(String factTimeStamp) {
    -    SimpleDateFormat parser = new SimpleDateFormat(CarbonCommonConstants.CARBON_TIMESTAMP_MILLIS);
    -    Date dateToStr = null;
         try {
    -      dateToStr = parser.parse(factTimeStamp);
    -      return dateToStr.getTime();
    -    } catch (ParseException e) {
    -      LOGGER.error("Cannot convert" + factTimeStamp + " to Time/Long type value" + e.getMessage());
    -      parser = new SimpleDateFormat(CarbonCommonConstants.CARBON_TIMESTAMP);
    +      // for new loads the factTimeStamp will be long string
    --- End diff --
   
    Do not mention "new loads" in line 275. Add comment in line 279, mentioning it is the processing for existing table before carbon 1.3


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1666#discussion_r157420051
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/LoadMetadataDetails.java ---
    @@ -241,26 +248,28 @@ private long convertTimeStampToLong(String factTimeStamp) {
        * @return
        */
       public Long getTimeStamp(String loadStartTime) {
    -    Date dateToStr = null;
         try {
    -      dateToStr = parser.parse(loadStartTime);
    -      return dateToStr.getTime() * 1000;
    -    } catch (ParseException e) {
    -      LOGGER.error("Cannot convert" + loadStartTime + " to Time/Long type value" + e.getMessage());
    -      return null;
    +      // for new loads the factTimeStamp will be long string
    --- End diff --
   
    Do not mention "new loads" in line 275. Add comment in line 279, mentioning it is the processing for existing table before carbon 1.3


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1666#discussion_r157420301
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/LoadMetadataDetails.java ---
    @@ -211,25 +234,32 @@ public long getLoadStartTimeAsLong() {
        * @return
        */
       private long convertTimeStampToLong(String factTimeStamp) {
    --- End diff --
   
    What is the difference between this function and getTimeStamp, seems they are doing the same thing


---