[AMORO-3632]: data-retention, add support for partition column type Date #3665
Conversation
OK, no problem |
@zhoujinsong I have added a new test case. Can you review the code to see if there is any problem? tks! |
| case STRING: | ||
| return LocalDateTime.ofInstant( | ||
| Instant.ofEpochMilli(expireTimestamp), getDefaultZoneId(field)) | ||
| .format( |
There was a problem hiding this comment.
Could you please help to add some comments for this function, thanks
| int compared = filePartitionValue.compareTo(partitionUpperBound); | ||
| Boolean compareResult = | ||
| expireField.type() == Types.StringType.get() ? compared <= 0 : compared < 0; | ||
| Boolean compareResult; |
There was a problem hiding this comment.
why do we add DateType to the special case? if the bound is the problem, could we calculate the bound with `(int) ((expireTimestamp + 24 * 60 * 60 * 1000) + / (24 * 60 * 60 * 1000))
There was a problem hiding this comment.
I am re-adjusting the date boundary value calculation problem. This part still maintains the original string type calculation logic.
| expected = | ||
| Lists.newArrayList( | ||
| createRecord(2, "222", parseMillis("2022-01-03T12:00:00"), "2022-01-03T12:00:00")); | ||
| } else if (expireByDate()) { |
There was a problem hiding this comment.
Could you please add some more tests to cover the bound case(eg, 2022-01-03T23:59:59, 2022-01-03T00:00:00, 2022-01-03T00:00:01)
|
@klion26 @zhoujinsong teachers，Could you please help me look at the code again? I have added a few boundary test cases, but there are still some problems during the test. I am a little confused. test command： mvn test -Dtest=TestDataExpire#testDateTypeBoundaryConditions* -pl amoro-ams |
| } | ||
|
|
||
| /** | ||
| * Convert expiration timestamp to appropriate value based on field type. This method handles |
There was a problem hiding this comment.
I tried to format the comment here to make it more readable:
/**
* Convert expiration timestamp to the appropriate value based on field type.
*
* <p>This method handles different field types for data expiration:
*
* <ul>
* <li>TIMESTAMP: converts milliseconds to microseconds
* <li>LONG: handles both millisecond and second formats
* <li>STRING: formats timestamp as date string using configured pattern
* <li>DATE: converts timestamp to days since epoch
* </ul>
*
* @param expirationConfig expiration configuration containing format patterns
* @param field the field being used for expiration
* @param expireTimestamp timestamp in milliseconds for expiration boundary
* @return comparable value appropriate for the field type
*/
|
Besides, there seem to be some unit test errors that need to be fixed. @lsyulong |
|
@lsyulong do you still have time to fix the issues in CI or can I help you fix it if you need? |
@zhoujinsong Sorry, due to recent time issues, this issue has been put on hold. Can you help fix other issues? I need your help. |
|
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. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
3 participants
Why are the changes needed?
Close #3632
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