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

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[manishgupta88]

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 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


---

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

[manishgupta88]

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 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


---

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

[dhatchayani]

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 
Date:   2018-07-20T14:36:00Z

[CARBONDATA-2753] Fix Compatibility issues on index Files




---