[GitHub] carbondata pull request #2534: [CARBONDATA-2753] Fix Compatibility issues on...

[qiuchenjian-2]

GitHub user dhatchayani opened a pull request:

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

    [CARBONDATA-2753] Fix Compatibility issues on index Files

    **Problem:**
    Currently,in the segmentFile we are writing the index files list in files field, only if it exists, otherwise it will be empty(in case if it is merged to merge index file). But in the old store, we were writing both the files and mergeFileName fields even if the index files are merged.
   
    **Solution:**
    While querying we have to check the physical existence of the index files listed in the files field. If it physically exists, then we have to consider that.
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [x] Testing done
            Manual Testing with Old Store
           
     - [ ] 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/dhatchayani/carbondata CARBONDATA-2753_indexFiles

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

    https://github.com/apache/carbondata/pull/2534.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 #2534
   
----
commit e257177c182783fb6c78507d4d8ad9823022acb6
Author: dhatchayani <dhatcha.official@...>
Date:   2018-07-20T14:36:00Z

    [CARBONDATA-2753] Fix Compatibility issues on index Files

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6128/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6131/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6133/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    retest sdv please



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2534#discussion_r204287026
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java ---
    @@ -597,7 +597,14 @@ public SegmentFile getSegmentFile() {
               Set<String> files = entry.getValue().getFiles();
               if (null != files && !files.isEmpty()) {
                 for (String indexFile : files) {
    -              indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
    +              String indexFilePath = location + CarbonCommonConstants.FILE_SEPARATOR + indexFile;
    +              try {
    +                if (FileFactory.isFileExist(indexFilePath)) {
    +                  indexFiles.put(indexFilePath, null);
    +                }
    +              } catch (IOException e) {
    +                LOGGER.error(e.getMessage());
    --- End diff --
   
    1. Don't LOG the error here, throw IO exception back to caller
    2. Mention the compatibility version number and provide a detailed comment in this method for file Existence check including the version numbers


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2534#discussion_r204287054
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java ---
    @@ -597,7 +597,14 @@ public SegmentFile getSegmentFile() {
               Set<String> files = entry.getValue().getFiles();
               if (null != files && !files.isEmpty()) {
                 for (String indexFile : files) {
    -              indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
    +              String indexFilePath = location + CarbonCommonConstants.FILE_SEPARATOR + indexFile;
    +              try {
    +                if (FileFactory.isFileExist(indexFilePath)) {
    +                  indexFiles.put(indexFilePath, null);
    +                }
    +              } catch (IOException e) {
    +                LOGGER.error(e.getMessage());
    --- End diff --
   
    1. Don't LOG the error here, throw IO exception back to caller
    2. Mention the compatibility version number and provide a detailed comment in this method for file Existence check including the version numbers


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2534
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6146/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---