[CARBONDATA-3560] Fixed issues for Add Segment #3426

manishnalla1994 · 2019-10-30T05:59:43Z

Issue1 : When the format is given in uppercase, add segment fails with unknown format.
Solution1 : Made format case-insensitive.

Issue2 : The same path is being added repeatedly, blocked this operation.

Issue3 : Added validation for the folder not containing carbon files.

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.

CarbonDataQA · 2019-10-30T06:21:59Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/713/

CarbonDataQA · 2019-10-30T07:42:53Z

Build Failed with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/721/

CarbonDataQA · 2019-10-30T07:48:09Z

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/720/

CarbonDataQA · 2019-10-30T09:27:02Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/714/

CarbonDataQA · 2019-10-30T10:37:14Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/721/

CarbonDataQA · 2019-10-30T10:47:36Z

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

manishnalla1994 · 2019-10-30T13:01:48Z

@ravipesala Please review.

jackylk · 2019-10-31T15:12:56Z

core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java

+   * @param segmentPath
+   * @return
+   */
+  public static CarbonFile[] getListOfCarbonIndexFiles(String segmentPath) {


Is the input parameter point to segment folder of transactional table?

Yes, its the path to segment folder to get the carbon file.

jackylk · 2019-10-31T15:13:26Z

core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java

+    CarbonFile segmentFolder = FileFactory.getCarbonFile(segmentPath);
+    CarbonFile[] indexFiles = segmentFolder.listFiles(new CarbonFileFilter() {
+      @Override public boolean accept(CarbonFile file) {
+        return (file.getName().endsWith(CarbonTablePath.INDEX_FILE_EXT) || file.getName()


move file.getName() to next line

jackylk · 2019-10-31T15:16:42Z

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

+
+    val allSegments = SegmentStatusManager.readLoadMetadata(carbonTable.getMetadataPath)
+
+    for (currSegment <- allSegments) {


use allSegments.contains or .exist instead of for

add comment to explain this validation

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

jackylk · 2019-10-31T15:18:46Z

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

@@ -92,6 +92,22 @@ case class CarbonAddLoadCommand(
    val segmentPath = options.getOrElse(
      "path", throw new UnsupportedOperationException("PATH is manadatory"))

+    val format = options.getOrElse("format", "carbondata")
+    val isCarbonFormat = format.equals("carbondata") || format.equals("carbon")


move to line 107

kunal642 · 2019-11-02T16:40:10Z

...est/src/test/scala/org/apache/carbondata/spark/testsuite/addsegment/AddSegmentTestCase.scala

@@ -542,7 +541,7 @@ class AddSegmentTestCase extends QueryTest with BeforeAndAfterAll {
    copy(path.toString, newPath)
    checkAnswer(sql("select count(*) from addsegment1"), Seq(Row(30)))

-    sql(s"alter table addsegment1 add segment options('path'='$newPath', 'format'='parquet')").show()
+    sql(s"alter table addsegment1 add segment options('path'='$newPath', 'format'='PARQUET')").show()


why is this change required?

This change is required just for the testing purpose of uppercase format, so that I need not add a new testcase just for case sensitivity.

CarbonDataQA · 2019-11-04T04:56:29Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/725/

CarbonDataQA · 2019-11-04T06:14:03Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/732/

CarbonDataQA · 2019-11-04T06:16:12Z

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

CarbonDataQA · 2019-11-04T07:25:40Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/730/

ravipesala · 2019-11-04T08:00:41Z

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

+    val isCarbonFormat = format.equalsIgnoreCase("carbondata") || format.equalsIgnoreCase("carbon")
+
+    // If in the given location no carbon files are found then we can throw an exception
+    if (isCarbonFormat && SegmentFileStore.getListOfCarbonIndexFiles(segmentPath).isEmpty) {


I think this validation should come from SparkFiileFormat, so ideally there should not be any changes in this class for format level validation. Better move it to format level

@ravipesala Here we are just checking the presence of carbon files present in the current folder location and as there was a 'isCarbonFormat' below so I just shifted it above and used it.

If we need to check this, better to check weather index file and data file are matching:

If it has merge index file, all data file should be present in merge index file

otherwise, one index file for one data file should present
please move this validation to another function and invoke it here

@manishnalla1994 the condition earlier is not for validation, it is just to update the metadata. How is the behavior of SparkFiileFormat infer schema if there are no carbondata or carbonindex files? I feel it is better to correct in the down layer than adding validations on top layer.

CarbonDataQA · 2019-11-04T08:25:58Z

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/737/

CarbonDataQA · 2019-11-04T08:26:47Z

Build Failed with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/738/

CarbonDataQA · 2019-11-04T12:16:00Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/734/

CarbonDataQA · 2019-11-04T13:27:38Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/740/

CarbonDataQA · 2019-11-04T15:49:53Z

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

CarbonDataQA · 2019-11-05T06:32:08Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/746/

jackylk · 2019-11-05T06:42:34Z

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

+    if (allSegments.exists(a =>
+      a.getPath != null && a.getPath.equalsIgnoreCase(segmentPath)
+    )) {
+      throw new AnalysisException(s"Cannot add the segment. This path is already in use by " +


change message to path already exists in table status file, can not add same segment path repeatedly: $path

jackylk · 2019-11-05T06:43:07Z

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala

+    val format = options.getOrElse("format", "carbondata")
+    val isCarbonFormat = format.equalsIgnoreCase("carbondata") || format.equalsIgnoreCase("carbon")
+
+    // If in the given location no carbon files are found then we can throw an exception


Suggested change

// If in the given location no carbon files are found then we can throw an exception

// If in the given location no carbon index files are found then we should throw an exception

CarbonDataQA · 2019-11-05T07:26:31Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/747/

CarbonDataQA · 2019-11-05T08:37:07Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/753/

CarbonDataQA · 2019-11-05T09:00:21Z

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

CarbonDataQA · 2019-11-07T11:41:05Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/769/

CarbonDataQA · 2019-11-07T12:53:00Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/775/

CarbonDataQA · 2019-11-07T12:56:03Z

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

ravipesala · 2019-11-11T08:16:59Z

LGTM

manishnalla1994 · 2019-11-11T08:38:52Z

retest this please

CarbonDataQA · 2019-11-11T09:04:00Z

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/785/

CarbonDataQA · 2019-11-11T10:12:18Z

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/791/

CarbonDataQA · 2019-11-11T10:40:46Z

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

ajantha-bhat · 2019-11-11T11:27:55Z

LGTM

manishnalla1994 force-pushed the AddSegmentFixes branch 2 times, most recently from 633553d to 7f032f1 Compare October 30, 2019 06:03

manishnalla1994 force-pushed the AddSegmentFixes branch from 7f032f1 to 09d2e5c Compare October 30, 2019 09:08

jackylk reviewed Oct 31, 2019

View reviewed changes

.../src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala Show resolved Hide resolved

jackylk reviewed Oct 31, 2019

View reviewed changes

kunal642 reviewed Nov 2, 2019

View reviewed changes

manishnalla1994 force-pushed the AddSegmentFixes branch 3 times, most recently from 6a84526 to 44f9d68 Compare November 4, 2019 06:50

ravipesala reviewed Nov 4, 2019

View reviewed changes

manishnalla1994 force-pushed the AddSegmentFixes branch from 44f9d68 to 1b3f616 Compare November 4, 2019 11:50

manishnalla1994 force-pushed the AddSegmentFixes branch from 1b3f616 to 66c584f Compare November 5, 2019 06:15

jackylk reviewed Nov 5, 2019

View reviewed changes

manishnalla1994 force-pushed the AddSegmentFixes branch from 66c584f to 10c2a5f Compare November 5, 2019 07:06

Fixed issues for Add Segment

b0389d5

manishnalla1994 force-pushed the AddSegmentFixes branch from 10c2a5f to b0389d5 Compare November 7, 2019 11:20

asfgit closed this in 9aa5973 Nov 11, 2019


		val allSegments = SegmentStatusManager.readLoadMetadata(carbonTable.getMetadataPath)

		for (currSegment <- allSegments) {

	// If in the given location no carbon files are found then we can throw an exception
	// If in the given location no carbon index files are found then we should throw an exception

[CARBONDATA-3560] Fixed issues for Add Segment #3426

[CARBONDATA-3560] Fixed issues for Add Segment #3426

Conversation

manishnalla1994 commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

CarbonDataQA commented Oct 30, 2019

manishnalla1994 commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 4, 2019

CarbonDataQA commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Nov 5, 2019

CarbonDataQA commented Nov 5, 2019

CarbonDataQA commented Nov 5, 2019

CarbonDataQA commented Nov 7, 2019

CarbonDataQA commented Nov 7, 2019

CarbonDataQA commented Nov 7, 2019

ravipesala commented Nov 11, 2019

manishnalla1994 commented Nov 11, 2019

CarbonDataQA commented Nov 11, 2019

CarbonDataQA commented Nov 11, 2019

CarbonDataQA commented Nov 11, 2019

ajantha-bhat commented Nov 11, 2019