-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CARBONDATA-4270]Delete segment expect remain_number #4203
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
start build |
retest this please |
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
Outdated
Show resolved
Hide resolved
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
Outdated
Show resolved
Hide resolved
val ex = intercept[Exception] { | ||
sql("delete from table deleteSegmentTable expect segment.remain_number = -1") | ||
} | ||
assert(ex.getMessage.contains("not found in database")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"not found in database" has nothing to do with -1, why choose this msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
...rg/apache/carbondata/spark/testsuite/deletesegment/DeleteSegmentByRemainNumberTestCase.scala
Show resolved
Hide resolved
sql( | ||
s"""LOAD DATA local inpath '$resourcesPath/dataretention3.csv' | ||
| INTO TABLE deleteSegmentTable OPTIONS('DELIMITER'= ',', 'QUOTECHAR'= '"')""".stripMargin) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need some after all function to make sure the table has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have define function "beforeEach" to drop table
// if insert overwrite in progress, do not allow delete segment | ||
if (SegmentStatusManager.isOverwriteInProgressInTable(carbonTable)) { | ||
throw new ConcurrentOperationException(carbonTable, "insert overwrite", "delete segment") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this pr do not support insert overwrite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table cannot be updated when deleted
} | ||
|
||
val segments = CarbonStore.readSegments(carbonTable.getTablePath, showHistory = false, None) | ||
if (segments.length == remaining.toInt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remaining.toInt means the segment count shoule be int range, does this restriction necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there has some segment with status SegmentStatus.MARKED_FOR_DELETE, does this condition is right or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interger value from -2147483648 to 2147483647. i think the remaining number of segment will not exceed this range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SegmentStatus.MARKED_FOR_DELETE won't be in remaining range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remaining.toInt can throw java.lang.NumberFormatException , handle appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. i have modify sql parse code. transfer 'remain_number' to Integer value directly.
retest this please |
jenkins,add to whitelist |
add to whitelist |
retest this please |
@CarbonDataQA2 why this pr can not trigger the CI, please help. |
@CarbonDataQA2 why this pr can not trigger the CI, please help. |
add to whitelist |
} | ||
|
||
// Through the remaining number, get the delete id | ||
val deleteSegmentIds = segments.filter(segment => segment.getSegmentStatus != SegmentStatus.MARKED_FOR_DELETE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of filtering out MFD segments, can only take success and compacted segments and avoid In-progress segments, because clean files will anyways take care of those segments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have modify this problem
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/263/ |
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5860/ |
Hi @W1thOut , can you raise the discussion in the community first? As you want to expose a new DDL, please check the community guidelines here: https://www.mail-archive.com/[email protected]/msg01835.html |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4117/ |
override def beforeEach(): Unit = { | ||
initTestTable | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test cases with SI as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. i have add test cases with SI
retest this please |
1 similar comment
retest this please |
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5865/ |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4123/ |
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/268/ |
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/269/ |
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5866/ |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4124/ |
okay. but i can not access community: http://apache-carbondata-mailing-list-archive.1130556.n5.nabble.com/ |
http://apache-carbondata-dev-mailing-list-archive.168.s1.nabble.com/ |
retest this please |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4127/ |
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/272/ |
retest this please |
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5871/ |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4129/ |
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/274/ |
Revise modify code add SI test add no partition table teset code checkstyle
retest this please |
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4133/ |
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5876/ |
Build Success with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/278/ |
Could you give me your valuable comments/inputs/suggestions. |
does this pr can be a new kind of segment management. |
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4383/ |
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/6126/ |
Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/516/ |
Why is this PR needed?
In some scenarios, need to delete old segments in batches and keep only latest few segments.
What changes were proposed in this PR?
Add a DML.
Does this PR introduce any user interface change?
Is any new testcase added?