Skip to content

Commit 75ae6e8

Browse files
committed
[python] Address review feedback for partition expiration
- Fix dead code in _parse_with_formatter: fallback now strips time directives and retries as date-only, matching Java's LocalDate.parse - Replace fragile str.replace() with regex tokenizer for Java→Python format conversion to avoid overlapping token issues - Skip partitions with last_file_creation_time=0 in update-time strategy to prevent false expiration of unknown-state partitions - Remove unrelated statistics()/analyze() methods from file_store_table - Fix _parse_duration type hint to Optional[str] - Add tests for date-only fallback and zero-creation-time guard
1 parent 62ceef9 commit 75ae6e8

5 files changed

Lines changed: 79 additions & 74 deletions

File tree

paimon-python/pypaimon/partition/partition_expire.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ def _read_partition_entries(table) -> List[PartitionEntry]:
292292
]
293293

294294

295-
def _parse_duration(duration_str: str) -> Optional[timedelta]:
295+
def _parse_duration(duration_str: Optional[str]) -> Optional[timedelta]:
296296
"""
297297
Parse a duration string like '7d', '1h', '30m', '10s', '1d 2h'.
298298
Also supports ISO-like formats: '7 days', '1 hour'.

paimon-python/pypaimon/partition/partition_expire_strategy.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,14 @@ def select_expired_partitions(
150150
self, partition_entries: List[PartitionEntry], expiration_time: datetime
151151
) -> List[PartitionEntry]:
152152
expiration_millis = int(expiration_time.timestamp() * 1000)
153-
return [
154-
entry for entry in partition_entries
155-
if expiration_millis > entry.last_file_creation_time
156-
]
153+
expired = []
154+
for entry in partition_entries:
155+
if entry.last_file_creation_time == 0:
156+
logger.warning(
157+
"Partition %s has last_file_creation_time=0 (unknown), skipping expiration.",
158+
entry.spec,
159+
)
160+
continue
161+
if expiration_millis > entry.last_file_creation_time:
162+
expired.append(entry)
163+
return expired

paimon-python/pypaimon/partition/partition_time_extractor.py

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -37,32 +37,41 @@
3737
"%Y-%m-%d",
3838
]
3939

40-
# Java DateTimeFormatter → Python strptime conversion (order matters: longest first)
41-
_JAVA_TO_PYTHON_PATTERNS = [
42-
("yyyy", "%Y"),
43-
("yy", "%y"),
44-
("MM", "%m"),
45-
("dd", "%d"),
46-
("HH", "%H"),
47-
("mm", "%M"),
48-
("SSS", "%f"),
49-
("SS", "%f"),
50-
("ss", "%S"),
51-
]
40+
# Java DateTimeFormatter → Python strptime token mapping.
41+
# Note: Python's %f is microseconds (6 digits); Java's SSS is milliseconds (3 digits).
42+
# This works for parsing because strptime(%f) accepts 1-6 digits, but the semantic
43+
# precision differs. Callers should be aware of this when formatting output.
44+
_JAVA_TO_PYTHON_TOKENS = {
45+
"yyyy": "%Y",
46+
"yy": "%y",
47+
"MM": "%m",
48+
"dd": "%d",
49+
"HH": "%H",
50+
"mm": "%M",
51+
"SSS": "%f",
52+
"SS": "%f",
53+
"ss": "%S",
54+
}
55+
56+
# Regex that matches Java tokens (longest first) or quoted literals
57+
_JAVA_TOKEN_RE = re.compile(
58+
r"'([^']*)'|(" + "|".join(re.escape(k) for k in sorted(_JAVA_TO_PYTHON_TOKENS, key=len, reverse=True)) + r")"
59+
)
5260

5361

5462
def _java_to_python_format(java_fmt: str) -> str:
5563
"""Convert Java DateTimeFormatter pattern to Python strptime format.
5664
57-
Handles:
58-
- Standard tokens: yyyy, MM, dd, HH, mm, ss, SSS
59-
- Quoted literals: 'T' → T (strips single quotes)
65+
Uses regex tokenization to avoid overlapping-replacement issues with
66+
sequential str.replace().
6067
"""
61-
import re
62-
# Strip Java quoted literals: 'T' → T, '' → '
63-
result = re.sub(r"'([^']*)'", r"\1", java_fmt)
64-
for java_pat, py_pat in _JAVA_TO_PYTHON_PATTERNS:
65-
result = result.replace(java_pat, py_pat)
68+
def _replace_token(m):
69+
if m.group(1) is not None:
70+
return m.group(1)
71+
return _JAVA_TO_PYTHON_TOKENS[m.group(2)]
72+
73+
# Replace known tokens; leave unmatched characters (literals like '-', ' ', 'T') as-is
74+
result = _JAVA_TOKEN_RE.sub(_replace_token, java_fmt)
6675
return result
6776

6877

@@ -129,12 +138,23 @@ def _to_local_datetime(self, timestamp_string: str) -> datetime:
129138

130139
@staticmethod
131140
def _parse_with_formatter(timestamp_string: str, formatter: str) -> datetime:
132-
"""Parse using the converted Python strptime pattern."""
141+
"""Parse using the converted Python strptime pattern.
142+
143+
Mirrors Java behavior: tries LocalDateTime.parse first, then falls back
144+
to LocalDate.parse (date-only) with the same formatter pattern.
145+
"""
133146
try:
134147
return datetime.strptime(timestamp_string, formatter)
135148
except ValueError:
136-
parsed_date = datetime.strptime(timestamp_string, formatter).date()
137-
return datetime.combine(parsed_date, dt_time.min)
149+
# Fallback: strip time directives (%H, %M, %S, %f) and surrounding
150+
# literals to get a date-only format, mirroring Java's LocalDate.parse fallback.
151+
date_only_format = re.split(r'(?=%H|%M|%S|%f)', formatter)[0].rstrip()
152+
# Also strip any trailing non-directive separators (e.g. trailing space or 'T')
153+
date_only_format = date_only_format.rstrip(' T-:')
154+
if date_only_format and date_only_format != formatter:
155+
parsed_date = datetime.strptime(timestamp_string, date_only_format).date()
156+
return datetime.combine(parsed_date, dt_time.min)
157+
raise
138158

139159
@staticmethod
140160
def _parse_default(timestamp_string: str) -> datetime:

paimon-python/pypaimon/table/file_store_table.py

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -395,52 +395,6 @@ def new_batch_write_builder(self) -> BatchWriteBuilder:
395395
def new_stream_write_builder(self) -> StreamWriteBuilder:
396396
return StreamWriteBuilder(self)
397397

398-
def statistics(self) -> 'Optional[Statistics]':
399-
"""Read existing statistics for this table from the latest snapshot.
400-
401-
Returns:
402-
A Statistics instance, or None if no statistics exist.
403-
"""
404-
from pypaimon.stats.stats_file_handler import StatsFileHandler
405-
snapshot_mgr = self.snapshot_manager()
406-
snapshot = snapshot_mgr.get_latest_snapshot()
407-
if snapshot is None:
408-
return None
409-
handler = StatsFileHandler(self.file_io, self.table_path)
410-
return handler.read_stats(snapshot)
411-
412-
def analyze(self, columns: 'Optional[List[str]]' = None) -> 'Statistics':
413-
"""Compute column-level statistics from manifest metadata.
414-
415-
This method scans manifest file entries to aggregate per-file statistics
416-
(min, max, null_count) into table-level column statistics. It does NOT
417-
scan actual data files, making it very fast.
418-
419-
Args:
420-
columns: Optional list of column names to analyze. If None, all
421-
columns are analyzed.
422-
423-
Returns:
424-
A Statistics instance with the computed stats.
425-
426-
Raises:
427-
ValueError: If the table has no snapshots or specified columns
428-
do not exist.
429-
"""
430-
from pypaimon.stats.statistics_collector import StatisticsCollector
431-
from pypaimon.stats.stats_file_handler import StatsFileHandler
432-
433-
collector = StatisticsCollector(self)
434-
stats = collector.collect(columns=columns)
435-
if stats is None:
436-
raise ValueError("Cannot analyze table: no snapshots exist.")
437-
438-
# Write statistics file
439-
handler = StatsFileHandler(self.file_io, self.table_path)
440-
handler.write_stats(stats)
441-
442-
return stats
443-
444398
def new_full_text_search_builder(self) -> 'FullTextSearchBuilder':
445399
from pypaimon.table.source.full_text_search_builder import FullTextSearchBuilderImpl
446400
return FullTextSearchBuilderImpl(self)

paimon-python/pypaimon/tests/partition_expire_test.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,12 @@ def test_extract_single_digit_month_day(self):
8181
result = extractor.extract(["dt"], ["2024-1-5"])
8282
self.assertEqual(result, datetime(2024, 1, 5, 0, 0, 0))
8383

84+
def test_formatter_date_only_fallback(self):
85+
"""Formatter with time components should fall back to date-only parsing."""
86+
extractor = PartitionTimeExtractor(formatter="yyyy-MM-dd HH:mm:ss")
87+
result = extractor.extract(["dt"], ["2024-03-15"])
88+
self.assertEqual(result, datetime(2024, 3, 15, 0, 0, 0))
89+
8490

8591
class TestPartitionValuesTimeExpireStrategy(unittest.TestCase):
8692
"""Tests for PartitionValuesTimeExpireStrategy."""
@@ -183,6 +189,24 @@ def test_no_expired_when_all_recent(self):
183189
expired = strategy.select_expired_partitions(entries, expiration_time)
184190
self.assertEqual(len(expired), 0)
185191

192+
def test_zero_creation_time_skipped(self):
193+
"""Partitions with last_file_creation_time=0 should be skipped (unknown state)."""
194+
strategy = PartitionUpdateTimeExpireStrategy(
195+
partition_keys=["dt"],
196+
)
197+
now = datetime(2024, 6, 1)
198+
old_time_millis = int((now - timedelta(days=30)).timestamp() * 1000)
199+
200+
entries = [
201+
PartitionEntry(spec={"dt": "2024-05-01"}, last_file_creation_time=old_time_millis),
202+
PartitionEntry(spec={"dt": "2024-04-01"}, last_file_creation_time=0),
203+
]
204+
205+
expiration_time = now - timedelta(days=7)
206+
expired = strategy.select_expired_partitions(entries, expiration_time)
207+
self.assertEqual(len(expired), 1)
208+
self.assertEqual(expired[0].spec["dt"], "2024-05-01")
209+
186210

187211
class TestPartitionExpire(unittest.TestCase):
188212
"""Tests for PartitionExpire orchestration class."""

0 commit comments

Comments
 (0)