hudi-agent commented on code in PR #19081: URL: https://github.com/apache/hudi/pull/19081#discussion_r3485076550
########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/ttl/strategy/KeepByEventTimeStrategy.java: ########## @@ -0,0 +1,193 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hudi.table.action.ttl.strategy; + +import org.apache.hudi.common.table.timeline.HoodieInstantTimeGenerator; +import org.apache.hudi.common.util.PartitionPathEncodeUtils; +import org.apache.hudi.table.HoodieTable; + +import lombok.extern.slf4j.Slf4j; + +import java.text.ParseException; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeParseException; +import java.time.temporal.TemporalAccessor; +import java.util.List; +import java.util.stream.Collectors; + +/** + * Event-time based partition TTL strategy. + * <p> + * Unlike {@link KeepByTimeStrategy} (which keys off the partition's last commit time) and + * {@link KeepByCreationTimeStrategy} (which keys off the partition creation commit time), this + * strategy derives the partition's reference time from the partition path itself. This makes TTL + * decoupled from write behaviour: late-arriving data into an old partition will not extend its + * lifetime, and backfilled historic partitions are still considered old. + * <p> + * First-version scope of supported partition path shapes: + * <ul> + * <li>Day: {@code 2026-04-24} / {@code dt=2026-04-24} / {@code 20260424} / {@code dt=20260424}</li> + * <li>Hour: {@code 2026-04-05/06} / {@code dt=2026-04-05/hh=06} / {@code 20260405/06} / {@code dt=20260405/hh=06}</li> + * </ul> + * The time block may appear at any contiguous segment range of the path. Examples like + * {@code eventType=login/20250405/05} are supported by setting + * {@link org.apache.hudi.config.HoodieTTLConfig#EVENT_TIME_PARTITION_START_INDEX} to {@code 1}. + */ +@Slf4j +public class KeepByEventTimeStrategy extends KeepByTimeStrategy { + + private final String eventTimeFormat; + private final int startIndex; + private final boolean deleteHiveDefaultPartition; + private final boolean hiveStylePartitioning; + + public KeepByEventTimeStrategy(HoodieTable hoodieTable, String instantTime) { + super(hoodieTable, instantTime); + this.eventTimeFormat = writeConfig.getPartitionTTLEventTimeFormat(); + this.startIndex = writeConfig.getPartitionTTLEventTimePartitionStartIndex(); + this.deleteHiveDefaultPartition = writeConfig.shouldDeleteHiveDefaultPartitionForEventTimeTTL(); + // Hive-style partitioning is a table-level property recorded at table creation; trust it + // rather than guessing per-segment from a stray '=' character in a value. + this.hiveStylePartitioning = Boolean.parseBoolean( + hoodieTable.getMetaClient().getTableConfig().getHiveStylePartitioningEnable()); + if (startIndex < 0) { + throw new IllegalArgumentException( + "hoodie.partition.ttl.strategy.event.time.partition.start.index must be >= 0, got " + startIndex); + } + } + + @Override + protected List<String> getExpiredPartitionsForTimeStrategy(List<String> partitionPathsForTTL) { + long cutoffMillis = resolveCutoffMillis(instantTime, ttlInMilis); Review Comment: 🤖 The event time here is parsed in fixed UTC, but the cutoff comes from `parseDateFromInstantTime`, which interprets the instant in `ZoneId.systemDefault()`. So for a JVM in, say, UTC+8 the two sides are compared across different zones. For day-granularity TTL that's a boundary effect, but for the supported hour granularity it can shift expiry by the full UTC offset. Should the event-time zone be configurable (or tied to the table timeline timezone) rather than hardcoded UTC? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/ttl/strategy/KeepByEventTimeStrategy.java: ########## @@ -0,0 +1,193 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hudi.table.action.ttl.strategy; + +import org.apache.hudi.common.table.timeline.HoodieInstantTimeGenerator; +import org.apache.hudi.common.util.PartitionPathEncodeUtils; +import org.apache.hudi.table.HoodieTable; + +import lombok.extern.slf4j.Slf4j; + +import java.text.ParseException; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeParseException; +import java.time.temporal.TemporalAccessor; +import java.util.List; +import java.util.stream.Collectors; + +/** + * Event-time based partition TTL strategy. + * <p> + * Unlike {@link KeepByTimeStrategy} (which keys off the partition's last commit time) and + * {@link KeepByCreationTimeStrategy} (which keys off the partition creation commit time), this + * strategy derives the partition's reference time from the partition path itself. This makes TTL + * decoupled from write behaviour: late-arriving data into an old partition will not extend its + * lifetime, and backfilled historic partitions are still considered old. + * <p> + * First-version scope of supported partition path shapes: + * <ul> + * <li>Day: {@code 2026-04-24} / {@code dt=2026-04-24} / {@code 20260424} / {@code dt=20260424}</li> + * <li>Hour: {@code 2026-04-05/06} / {@code dt=2026-04-05/hh=06} / {@code 20260405/06} / {@code dt=20260405/hh=06}</li> + * </ul> + * The time block may appear at any contiguous segment range of the path. Examples like + * {@code eventType=login/20250405/05} are supported by setting + * {@link org.apache.hudi.config.HoodieTTLConfig#EVENT_TIME_PARTITION_START_INDEX} to {@code 1}. + */ +@Slf4j +public class KeepByEventTimeStrategy extends KeepByTimeStrategy { + + private final String eventTimeFormat; + private final int startIndex; + private final boolean deleteHiveDefaultPartition; + private final boolean hiveStylePartitioning; + + public KeepByEventTimeStrategy(HoodieTable hoodieTable, String instantTime) { + super(hoodieTable, instantTime); + this.eventTimeFormat = writeConfig.getPartitionTTLEventTimeFormat(); + this.startIndex = writeConfig.getPartitionTTLEventTimePartitionStartIndex(); + this.deleteHiveDefaultPartition = writeConfig.shouldDeleteHiveDefaultPartitionForEventTimeTTL(); + // Hive-style partitioning is a table-level property recorded at table creation; trust it + // rather than guessing per-segment from a stray '=' character in a value. + this.hiveStylePartitioning = Boolean.parseBoolean( + hoodieTable.getMetaClient().getTableConfig().getHiveStylePartitioningEnable()); + if (startIndex < 0) { + throw new IllegalArgumentException( + "hoodie.partition.ttl.strategy.event.time.partition.start.index must be >= 0, got " + startIndex); + } + } + + @Override + protected List<String> getExpiredPartitionsForTimeStrategy(List<String> partitionPathsForTTL) { + long cutoffMillis = resolveCutoffMillis(instantTime, ttlInMilis); + DateTimeFormatter formatter = DateTimeFormatter.ofPattern(eventTimeFormat).withZone(ZoneOffset.UTC); + int segCount = segmentCount(eventTimeFormat); + return partitionPathsForTTL.stream().parallel() + .filter(path -> isPartitionExpiredByEventTime( + path, formatter, startIndex, segCount, cutoffMillis, deleteHiveDefaultPartition, hiveStylePartitioning)) + .collect(Collectors.toList()); + } + + /** + * Number of '/'-separated path segments the configured format occupies. + * Example: {@code yyyy-MM-dd/HH} -> 2. + */ + static int segmentCount(String format) { + return format.split("/").length; + } + + /** + * Resolve the "now" reference timestamp from {@code instantTime} so the strategy stays + * idempotent across retries of the same replace commit. + */ + static long resolveCutoffMillis(String instantTime, long ttlInMillis) { + try { + return HoodieInstantTimeGenerator.parseDateFromInstantTime(instantTime).getTime() - ttlInMillis; + } catch (ParseException e) { + throw new IllegalStateException("Failed to parse instant time " + instantTime, e); + } + } + + /** + * Decide whether a partition path is expired. + * <p> + * Package-private so unit tests can exercise it directly without spinning up a HoodieTable. + */ + static boolean isPartitionExpiredByEventTime(String partitionPath, + DateTimeFormatter formatter, + int startIndex, + int segCount, + long cutoffMillis, + boolean deleteHiveDefaultPartition, + boolean hiveStylePartitioning) { + String[] segments = partitionPath.split("/"); Review Comment: 🤖 Since this throws from inside the `.stream().parallel().filter(...)` over all candidate partitions, a single partition path that doesn't match the configured format/start-index/hive-style aborts the entire TTL run, so no partitions get expired until the offending one is fixed. I see the fail-fast intent is deliberate, but could one legacy or manually-created odd partition permanently stall TTL for the whole table? Might it be safer to skip-with-WARN here and only fail fast on a clearly global misconfig (e.g. zero matches)? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> -- 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]
