huyuanfeng2018 commented on code in PR #3964:
URL: https://github.com/apache/amoro/pull/3964#discussion_r2564614152
##########
amoro-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java:
##########
@@ -729,6 +729,31 @@ protected ExpireFiles expiredFileScan(
return expiredFiles;
}
+ private static int calExpireDaysForDate(long expireTimestamp) {
+ // Round up to next day to ensure inclusive behavior for date boundaries
+ // This allows the existing < comparison to work correctly for Date types
+ int expireDays = (int) (expireTimestamp / (24 * 60 * 60 * 1000) + 1);
Review Comment:
It is recommended to define a static variable.
##########
amoro-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java:
##########
@@ -729,6 +729,31 @@ protected ExpireFiles expiredFileScan(
return expiredFiles;
}
+ private static int calExpireDaysForDate(long expireTimestamp) {
+ // Round up to next day to ensure inclusive behavior for date boundaries
+ // This allows the existing < comparison to work correctly for Date types
+ int expireDays = (int) (expireTimestamp / (24 * 60 * 60 * 1000) + 1);
+ LOG.info("Date expiration: expireTimestamp={}, expireDays={}",
expireTimestamp, expireDays);
Review Comment:
This log should not be printed here, this is just a cal.
##########
amoro-ams/src/test/java/org/apache/amoro/server/optimizing/maintainer/TestDataExpire.java:
##########
@@ -637,4 +687,194 @@ private static DataExpirationConfig
parseDataExpirationConfig(MixedTable table)
Map<String, String> properties = table.properties();
return TableConfigurations.parseDataExpirationConfig(properties);
}
+
+ private boolean expireByDate() {
+ String expireField =
+ CompatiblePropertyUtil.propertyAsString(
+ getMixedTable().properties(),
TableProperties.DATA_EXPIRATION_FIELD, "");
+ Types.NestedField field = getMixedTable().schema().findField(expireField);
+ return field != null && field.type().typeId().equals(Type.TypeID.DATE);
+ }
+
+ @Test
+ public void testDateTypeBoundaryConditions() {
+ assumeTrue(expireByDate());
+ System.out.println(
Review Comment:
Same.
##########
amoro-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/IcebergTableMaintainer.java:
##########
@@ -790,6 +818,9 @@ protected static Expression getDataExpression(
DateTimeFormatter.ofPattern(
expirationConfig.getDateTimePattern(),
Locale.getDefault()));
return Expressions.lessThanOrEqual(field.name(), expireDateTime);
+ case DATE:
+ int expireDays = calExpireDaysForDate(expireTimestamp);
Review Comment:
```
LocalDate expireDate = Instant.ofEpochMilli(expireTimestamp)
.atZone(getDefaultZoneId(field)) //
.toLocalDate();
// Add one day to make the comparison inclusive
// (since we use lessThan instead of lessThanOrEqual)
int expireDays = (int) expireDate.plusDays(1).toEpochDay();
return expireDays;
```
WDYT？
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
