[GitHub] [carbondata] akkio-97 opened a new pull request #4001: [WIP]

[GitBox]

akkio-97 opened a new pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001


    ### Why is this PR needed?
   
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-720622523






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r521116946



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -877,6 +877,10 @@ public SegmentFile getSegmentFile() {
         }
         if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
           String mergeFileName = entry.getValue().getMergeFileName();
+          // remove file separator if already present

Review comment:
       better to store the correct path while taking input to add segment command itself instead of adding a hack like this?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r524321124



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -877,6 +877,10 @@ public SegmentFile getSegmentFile() {
         }
         if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
           String mergeFileName = entry.getValue().getMergeFileName();
+          // remove file separator if already present

Review comment:
       The path stored is always correct. It depends on the user whether he is putting '/' in the end for the segment path or not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r524321124



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -877,6 +877,10 @@ public SegmentFile getSegmentFile() {
         }
         if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
           String mergeFileName = entry.getValue().getMergeFileName();
+          // remove file separator if already present

Review comment:
       The path stored is always correct. It depends on the user whether he is putting '/' in the end for the segment path or not. If he is putting then adding delimiter again is wrong. Hence avoided that.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r534681054



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -877,6 +877,10 @@ public SegmentFile getSegmentFile() {
         }
         if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
           String mergeFileName = entry.getValue().getMergeFileName();
+          // remove file separator if already present

Review comment:
       if the user is putting "/", while storing itself, can we remove that and store it, instead of skipping here ? because some other place if the stored value is used. again same code has to be there also




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r535160669



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -877,6 +877,10 @@ public SegmentFile getSegmentFile() {
         }
         if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
           String mergeFileName = entry.getValue().getMergeFileName();
+          // remove file separator if already present

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-737999904


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5044/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-738000543


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3286/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r535365097



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -89,8 +89,13 @@ case class CarbonAddLoadCommand(
       throw new ConcurrentOperationException(carbonTable, "insert overwrite", "add segment")
     }
 
-    val inputPath = options.getOrElse(
+    var givenPath = options.getOrElse(
       "path", throw new UnsupportedOperationException("PATH is mandatory"))
+    // remove file separator if already present
+    if (givenPath.charAt(givenPath.length - 1) == '/') {

Review comment:
       ```suggestion
       if (givenPath.charAt(givenPath.length - 1) == 'CarbonCommonConstans.FILE_SEPARATOR') {
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r535365912



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -89,8 +89,13 @@ case class CarbonAddLoadCommand(
       throw new ConcurrentOperationException(carbonTable, "insert overwrite", "add segment")
     }
 
-    val inputPath = options.getOrElse(
+    var givenPath = options.getOrElse(
       "path", throw new UnsupportedOperationException("PATH is mandatory"))
+    // remove file separator if already present
+    if (givenPath.charAt(givenPath.length - 1) == '/') {
+      givenPath = givenPath.substring(0, givenPath.length - 1)
+    }
+    val inputPath = givenPath

Review comment:
       can you modify current add segment testcase where one has / at the end and the one doesn't ? so, testcase will cover new code also




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r535857150



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -89,8 +89,13 @@ case class CarbonAddLoadCommand(
       throw new ConcurrentOperationException(carbonTable, "insert overwrite", "add segment")
     }
 
-    val inputPath = options.getOrElse(
+    var givenPath = options.getOrElse(
       "path", throw new UnsupportedOperationException("PATH is mandatory"))
+    // remove file separator if already present
+    if (givenPath.charAt(givenPath.length - 1) == '/') {

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-738643747


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3307/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-738643866


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5066/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#discussion_r535951357



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -89,8 +89,13 @@ case class CarbonAddLoadCommand(
       throw new ConcurrentOperationException(carbonTable, "insert overwrite", "add segment")
     }
 
-    val inputPath = options.getOrElse(
+    var givenPath = options.getOrElse(
       "path", throw new UnsupportedOperationException("PATH is mandatory"))
+    // remove file separator if already present
+    if (givenPath.charAt(givenPath.length - 1) == '/') {
+      givenPath = givenPath.substring(0, givenPath.length - 1)
+    }
+    val inputPath = givenPath

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001#issuecomment-738730701


   LGTM


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

asfgit closed pull request #4001:
URL: https://github.com/apache/carbondata/pull/4001


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]