[GitHub] carbondata pull request #2739: [CARBONDATA-2954]Fix error when create extern...

[qiuchenjian-2]

GitHub user shardul-cr7 opened a pull request:

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

    [CARBONDATA-2954]Fix error when create external table command fired if path already exists

    Problem : Creating a external table and providing a valid location having some empty directory and .carbondata files was giving "operation not allowed: invalid datapath provided" error.
   
    Solution: It was happening because if the location was having some empty directory getFilePathExternalFilePath method in carbonutil.java was returning null due to the presence of empty directory.So made a slight modification to prevent this problem.
   
   
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [x] Testing done
            manually tested.
           
     - [ ] 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/shardul-cr7/carbondata latestb05

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

    https://github.com/apache/carbondata/pull/2739.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 #2739
   
----
commit 6e75500e5e4a081b5be69cbbde5e586e13e07d9f
Author: shardul-cr7 <shardulsingh22@...>
Date:   2018-09-20T14:12:54Z

    [CARBONDATA-2954]Fix error when create external table command fired if path already exists

----


---

[qiuchenjian-2]

Github user shardul-cr7 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/375/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8623/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/553/



---

[qiuchenjian-2]

Github user shardul-cr7 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/383/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8631/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/561/



---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2739#discussion_r219531131
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java ---
    @@ -2226,7 +2226,11 @@ public static String getFilePathExternalFilePath(String path, Configuration conf
           if (dataFile.getName().endsWith(CarbonCommonConstants.FACT_FILE_EXT)) {
             return dataFile.getAbsolutePath();
           } else if (dataFile.isDirectory()) {
    -        return getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration);
    +        if (getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration) == null) {
    --- End diff --
   
    Please  add/modify existing test case to handle this scenario
   
    Also please test query with an empty folder. we have other places in the code where we recursively read.
    Review and handle if a problem exists in that flow.
   
    example:
    CarbonTable.getFirstIndexFile --> also has same issue
   
   
     


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/427/



---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2739#discussion_r219739583
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java ---
    @@ -2226,7 +2226,11 @@ public static String getFilePathExternalFilePath(String path, Configuration conf
           if (dataFile.getName().endsWith(CarbonCommonConstants.FACT_FILE_EXT)) {
             return dataFile.getAbsolutePath();
           } else if (dataFile.isDirectory()) {
    -        return getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration);
    +        if (getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration) == null) {
    --- End diff --
   
    Handled the review comment and added the test cases also for the scenarios.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/428/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/607/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8677/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2739#discussion_r219759571
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -261,7 +261,11 @@ private static CarbonFile getFirstIndexFile(CarbonFile tablePath) {
         CarbonFile[] carbonFiles = tablePath.listFiles();
         for (CarbonFile carbonFile : carbonFiles) {
           if (carbonFile.isDirectory()) {
    -        return getFirstIndexFile(carbonFile);
    +        if (getFirstIndexFile(carbonFile) == null) {
    --- End diff --
   
    Please add comments


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2739#discussion_r219759607
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java ---
    @@ -2230,7 +2230,11 @@ public static String getFilePathExternalFilePath(String path, Configuration conf
           if (dataFile.getName().endsWith(CarbonCommonConstants.FACT_FILE_EXT)) {
             return dataFile.getAbsolutePath();
           } else if (dataFile.isDirectory()) {
    -        return getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration);
    +        if (getFilePathExternalFilePath(dataFile.getAbsolutePath(), configuration) == null) {
    --- End diff --
   
    Please add comments


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2739
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/432/



---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

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


---