fix(search): avoid year-0 crashes in Chinese rolling-window temporal extraction - #2636
Conversation
benfrank241
left a comment
benfrank241
left a comment
There was a problem hiding this comment.
thanks for this — the root cause is right and i reproduced the original #2533 crash: 过去一年做了什么 with a reference date in year 1 hits add_years(ref, -1) → year 0 → ValueError('year 0 is out of range'), exactly the consolidation failure. and 过去一年 at year 1 now correctly degrades to no-constraint, year 2 still extracts. nice.
one thing blocks merge though: the guard isn't threaded all the way through. at line 962 (relative_year_fixed_day_match) you converted the d computation to add_days(...) (which returns None on underflow) but left the call as bare constraint(d, d) instead of safe_constraint(d, d) — so None still reaches constraint. it just trades one crash for another:
去年今天 ref year=1: AttributeError: 'NoneType' object has no attribute 'replace'
大前年今天 ref year=1,2,3: AttributeError
去年昨天 ref year=1: AttributeError
same failure mode (impossible year shift during consolidation search), so consolidation still breaks on these. the sibling relative_year_daypart_match block right above it already does the right thing with safe_constraint — line 962 just needs the same swap.
could you:
- swap
constraint(d, d)→safe_constraint(d, d)at line 962 - add a regression covering
去年今天(and ideally大前年今天) at a low reference year, asserting no-constraint rather than a crash - while you're in there, quick audit of the remaining bare
constraint(d, d)calls fed by a year-shifted base so we don't leave another one — the前晚|前夜one can also underflow at the absolute datetime.min boundary
once that's in i'll re-verify and merge.
|
Thanks — pushed the sibling coverage update in Changes made:
Checks run locally:
|
benfrank241
left a comment
benfrank241
left a comment
There was a problem hiding this comment.
ack — sibling coverage complete, verified. the three cases that still crashed on the first pass (去年今天, 大前年今天, 去年昨天 at ref year 1) now cleanly return NO_CONSTRAINT instead of AttributeError, add_days() degrades on underflow, and the 昨晚/前晚 daypart paths are on safe_constraint too. valid low-year windows still extract (去年今天 ref=2 → range), so no over-degrading. 382 pass locally. superseding my earlier changes-requested.
approving + kicked off CI.
…3217) (#3413) Consolidation recalls with stored fact text as the query, so a phrase like "十万年前" (100,000 years ago) hit unguarded offset arithmetic in extract_period — which runs BEFORE analyze()'s dateparser guard — and escaped as ValueError('year -97974 is out of range'), deterministically failing every recall and consolidation touching the bank. The three years observed in #3217 (-534, -974, -97974) are exactly now.year - {2560, 3000, 100000}: query-time arithmetic, not stored rows (Python datetimes can't represent them, so no bad date can reach the DB through asyncpg in the first place). Three layers, mirroring the #2636 add_years fix: - extract_temporal_constraint (the recall choke point) degrades any analyzer failure to 'no temporal signal' with a warning; analyze() itself stays strict so parser bugs still surface in tests. - chinese_temporal_periods: add_months/subtract_months are now bounds-checked like add_years (returning None, plumbed through every call site), and day/week offsets go through the overflow-guarded add_days instead of raw timedelta addition. - temporal_periods: an explicit month + year 0000 match returns NO_TEMPORAL_CONSTRAINT instead of crashing datetime().
2 participants
Fixes #2533.
Summary
ValueErrors covered by regression testsTests
uv run --frozen pytest tests/test_query_analyzer.py -quv run --frozen ruff format --check hindsight_api/engine/chinese_temporal_periods.py tests/test_query_analyzer.pyuv run --frozen ruff check hindsight_api/engine/chinese_temporal_periods.py tests/test_query_analyzer.pypython3 -m py_compile hindsight_api/engine/chinese_temporal_periods.pygit diff --check