[AMORO-3272] Complete data expiration based on partition information #3273

lintingbin · 2024-10-17T12:07:56Z

Why are the changes needed?

Close #3272.

Brief change log

When the expiration field is the partition field and the expiration level is partition, prioritize using the partition information of the datafile to expire the data files.
Modify the expected results of a test case, which is slightly different from the previous implementation. In partition expiration, date comparison is needed, and data files with the same date should not be expired.

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

github-actions · 2024-11-30T00:04:10Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

klion26

@lintingbin Thanks for the contribution, I left some comments, please take a look when you're free thanks.

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java

lintingbin · 2024-12-04T02:27:25Z

@klion26 I have already responded and made modifications. Please help review it again.

klion26 · 2024-12-04T07:12:27Z

@lintingbin thanks for rebasing the comments, the change LGTM. let's see if there are any more comments from the community.

Do we need to modify the corresponding documents?

lintingbin · 2024-12-04T07:55:21Z

@lintingbin thanks for rebasing the comments, the change LGTM. let's see if there are any more comments from the community.

Do we need to modify the corresponding documents?

There is no need to modify the documentation since there are no changes in the parameters.

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java

lintingbin · 2024-12-06T08:45:54Z

@XBaith I have refactored the code by removing some unnecessary parts and optimizing the naming of variables to enhance readability. I also added a check for cases where transform is Void. Please review the code again.

XBaith · 2024-12-06T08:56:10Z

@XBaith I have refactored the code by removing some unnecessary parts and optimizing the naming of variables to enhance readability. I also added a check for cases where transform is Void. Please review the code again.

I don't see any unit test for that case. Do you mean you test locally for Void transform?

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java

XBaith

I wrote some unit tests for a case involving expiring partitions after dropping the partition field. Unfortunately, the current procedure cannot handle this scenario correctly.

Here’s the partition level case:

I set the partition field as op_time and inserted some records.
Then, I removed the partition field before expiring the data.

Expected behavior: All records should be retained since the partition field has been removed.

Please fix this issue and add additional unit tests to cover similar scenarios. Thanks!

XBaith · 2024-12-07T14:00:06Z

I wrote some unit tests for a case involving expiring partitions after dropping the partition field. Unfortunately, the current procedure cannot handle this scenario correctly.

Sorry for the mistake; this bug existed prior to this PR. I will raise another PR to address it.

Additionally, we should add more unit tests to cover vulnerabilities as much as possible. Thanks for your contribution!

klion26

Thanks for the contribution, LGTM

XBaith

LGTM

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java

codecov-commenter · 2025-02-21T17:48:02Z

Codecov Report

Attention: Patch coverage is 73.17073% with 11 lines in your changes missing coverage. Please review.

Project coverage is 27.76%. Comparing base (63aab48) to head (d48a8d9).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
.../optimizing/maintainer/IcebergTableMaintainer.java	71.79%	7 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3273      +/-   ##
============================================
+ Coverage     21.59%   27.76%   +6.16%     
- Complexity     2353     3627    +1274     
============================================
  Files           431      603     +172     
  Lines         40347    49212    +8865     
  Branches       5711     6344     +633     
============================================
+ Hits           8712    13662    +4950     
- Misses        30903    34597    +3694     
- Partials        732      953     +221

Flag	Coverage Δ
core	`27.76% <73.17%> (?)`
trino	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lintingbin · 2025-02-26T00:00:49Z

@czy006 @XBaith Now that all the problems have been solved, please help to review the code again.

XBaith · 2025-02-26T03:19:42Z

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java

@@ -194,7 +196,8 @@ private void testUnKeyedPartitionLevel() {

    List<Record> expected;
    if (tableTestHelper().partitionSpec().isPartitioned()) {
-      if (expireByStringDate()) {
+      // retention time is 1 day, expire partitions that order than 2022-01-02
+      if (expireByStringDate() && isMetricsNotNone()) {


We should keep the same result even if metrics mode is none

XBaith · 2025-02-26T03:23:12Z

I made minor code adjustments to ensure that the expiration behavior remains consistent when the expire field is of string type, preventing any confusion for users

feature: data-expire by partition info

bc12dbc

github-actions bot added the module:ams-server Ams server module label Oct 17, 2024

zhoujinsong added 2 commits October 21, 2024 17:55

Merge branch 'master' into feature/data_expire_by_partition_info

f992434

Merge branch 'master' into feature/data_expire_by_partition_info

6c8d772

github-actions bot added the stale label Nov 30, 2024

Merge branch 'master' into feature/data_expire_by_partition_info

e3b2e0f

klion26 reviewed Dec 3, 2024

View reviewed changes

add comments

54c3700

github-actions bot removed the stale label Dec 4, 2024

Aireed added this to the Release 0.8.0 milestone Dec 4, 2024

Aireed mentioned this pull request Dec 4, 2024

Release-0.8.0 Roadmap #3048

Open

33 tasks

Merge branch 'master' into feature/data_expire_by_partition_info

86f6b00

XBaith reviewed Dec 5, 2024

View reviewed changes

Refactor the code to improve readability.

aba77d8

Merge branch 'master' into feature/data_expire_by_partition_info

49ecbdc

lintingbin commented Dec 6, 2024

View reviewed changes

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java Outdated Show resolved Hide resolved

XBaith requested changes Dec 7, 2024

View reviewed changes

XBaith mentioned this pull request Dec 7, 2024

[Hotfix] Fix expire data after dropping partition #3354

Closed

3 tasks

baiyangtx and others added 4 commits December 10, 2024 10:32

Merge branch 'master' into feature/data_expire_by_partition_info

e20e266

Refactor the code to improve readability.

c1ebdd2

Merge branch 'master' into feature/data_expire_by_partition_info

6f537bd

Merge branch 'master' into feature/data_expire_by_partition_info

5c32a8c

klion26 approved these changes Feb 10, 2025

View reviewed changes

lintingbin requested a review from XBaith February 19, 2025 03:29

XBaith approved these changes Feb 19, 2025

View reviewed changes

Merge branch 'master' into feature/data_expire_by_partition_info

9376ad1

czy006 reviewed Feb 21, 2025

View reviewed changes

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java Outdated Show resolved Hide resolved

XBaith mentioned this pull request Feb 21, 2025

Core: Apply correct metric configs in GenericAppenderFactory apache/iceberg#12366

Open

feature: add test cases

bea5ae1

lintingbin commented Feb 21, 2025

View reviewed changes

...-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java Outdated Show resolved Hide resolved

fix test error

d48a8d9

XBaith reviewed Feb 21, 2025

View reviewed changes

amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java Outdated Show resolved Hide resolved

XBaith mentioned this pull request Feb 25, 2025

[Hotfix] Set table properties in GenericTaskWriters #3442

Merged

3 tasks

lintingbin and others added 2 commits February 25, 2025 18:11

optimize code

9cc3446

Merge branch 'master' into feature/data_expire_by_partition_info

6ff7532

XBaith reviewed Feb 26, 2025

View reviewed changes

update

7aa078b

checkstyle

896a80e

czy006 changed the title ~~feature: data-expire by partition info~~ [AMORO-3272] data-expire by partition info Feb 26, 2025

czy006 approved these changes Feb 26, 2025

View reviewed changes

XBaith merged commit 2ff66e5 into apache:master Feb 26, 2025
4 checks passed

lintingbin deleted the feature/data_expire_by_partition_info branch February 26, 2025 06:10

zhoujinsong changed the title ~~[AMORO-3272] data-expire by partition info~~ [AMORO-3272] Complete data expiration based on partition information Feb 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMORO-3272] Complete data expiration based on partition information #3273

[AMORO-3272] Complete data expiration based on partition information #3273

lintingbin commented Oct 17, 2024

github-actions bot commented Nov 30, 2024

klion26 left a comment

lintingbin commented Dec 4, 2024

klion26 commented Dec 4, 2024

lintingbin commented Dec 4, 2024

lintingbin commented Dec 6, 2024

XBaith commented Dec 6, 2024

XBaith left a comment

XBaith commented Dec 7, 2024 •

edited

Loading

klion26 left a comment

XBaith left a comment

codecov-commenter commented Feb 21, 2025 •

edited

Loading

lintingbin commented Feb 26, 2025

XBaith Feb 26, 2025

XBaith commented Feb 26, 2025

[AMORO-3272] Complete data expiration based on partition information #3273

[AMORO-3272] Complete data expiration based on partition information #3273

Conversation

lintingbin commented Oct 17, 2024

Why are the changes needed?

Brief change log

How was this patch tested?

Documentation

github-actions bot commented Nov 30, 2024

klion26 left a comment

Choose a reason for hiding this comment

lintingbin commented Dec 4, 2024

klion26 commented Dec 4, 2024

lintingbin commented Dec 4, 2024

lintingbin commented Dec 6, 2024

XBaith commented Dec 6, 2024

XBaith left a comment

Choose a reason for hiding this comment

XBaith commented Dec 7, 2024 • edited Loading

klion26 left a comment

Choose a reason for hiding this comment

XBaith left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 21, 2025 • edited Loading

Codecov Report

lintingbin commented Feb 26, 2025

XBaith Feb 26, 2025

Choose a reason for hiding this comment

XBaith commented Feb 26, 2025

XBaith commented Dec 7, 2024 •

edited

Loading

codecov-commenter commented Feb 21, 2025 •

edited

Loading