feat(ask): bind relative-time filters to event time (v2.13.0) - #581
feat(ask): bind relative-time filters to event time (v2.13.0)#581seonghobae wants to merge 10 commits into
Conversation
Global Ask 어제/그저께/지난주 now compares coalesce(event_occurred_at, created_at) on the Seoul calendar. Clustered bulk imports keep yesterday's events retrievable. Cited evidence names Time axis so a click still opens that post (ADR 0168).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Cycle note 2026-08-24T12:24Z. Head Independent APPROVE is required. I will not self-approve or squash-merge this PR. Squash-merge only with an independent APPROVE on this exact head plus required checks success. Copilot review requested. seonghobae is currently the only collaborator, so a human independent APPROVE still has to come from outside this loop. |
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Copilot/human independent review requested. Do not treat this comment as APPROVE.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Event-time binding does not reach candidate discovery
The keyword candidate subqueries still order by created_at desc limit 32 per term; the event-time window is applied only in the final rows query. When a bulk import clusters created_at and a term has more than 32 matches, a post whose event fell yesterday can be dropped at the discovery limit before the event-time filter runs. Pre-existing, but it undercuts the PR goal for large clustered corpora.
Was this helpful? React with 👍 or 👎 to provide feedback.
| visible_rows = [ | ||
| row | ||
| for row in rows | ||
| if can_see_post(row) and row_matches_time_range(row, resolved_time_range) | ||
| ][:limit] |
There was a problem hiding this comment.
📝 Info: Time filter applied twice
gather_global_chat_sources bounds coalesce(event_occurred_at, created_at) in SQL and re-checks the same window in Python via row_matches_time_range. The Python pass is redundant in production but is what actually enforces the window for the test FakeConnections, which ignore SQL WHERE clauses. row_matches_time_range keeps rows whose clock is absent, which cannot occur in production where created_at is NOT NULL.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ilter-v2130 # Conflicts: # CHANGELOG.md # CLAUDE.md # frontend/package.json # pyproject.toml # uv.lock
The post-merge seeded_db fixture created source_post rows with event_occurred_at (scripts/seed_demo_data.py::insert_fixture_source_posts) but never applied migrations/0168_source_post_event_occurred_at.sql, so six backend/tests/test_api.py tests failed with UndefinedColumn. Wire the migration in alongside the other seeded_db migrations.
| visible_rows = [ | ||
| row | ||
| for row in rows | ||
| if can_see_post(row) and row_matches_time_range(row, resolved_time_range) | ||
| ][:limit] |
There was a problem hiding this comment.
🔍 Time filter drops lineage-context neighbors
The top keyword match is expanded through its post_lineage_edge neighbors for before/after context (post_chat_ingestion.py), then those neighbors pass through the same event-time filter (post_chat_ingestion.py). A narrow window like "어제" drops any neighbor whose event fell on another day, largely defeating the expansion. This matches ADR 0168's narrowing but conflicts with the expansion's stated purpose.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ilter-v2130 # Conflicts: # CHANGELOG.md # backend/app/post_chat_ingestion.py # backend/tests/test_api.py # frontend/package.json # pyproject.toml # uv.lock
| resolved_time_range[0] if resolved_time_range else None, | ||
| resolved_time_range[1] if resolved_time_range else None, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Time-only questions return the whole window
When a temporal question has no non-stopword keywords, candidate_ids is empty and the final query (post_chat_ingestion.py) uses it only for ordering, not filtering. Every visible post inside the resolved window is then returned up to limit, ordered by event time. Pre-existing behavior, unchanged here.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
…ilter-v2130 # Conflicts: # backend/tests/test_api.py # frontend/package.json # pyproject.toml # uv.lock
There was a problem hiding this comment.
📝 Info: Event-time filter narrows candidates, does not discover by date
The relative-time window bounds the keyword-ranked candidate set; it does not surface posts by date alone. '어제 무슨 일이 있었나요?' still needs the yesterday-event post to match a residual generic term (무슨/일이/있었나요) to enter candidacy. The test_global_sources_bind_relative_time_to_event_clock_not_ingest_cluster FakeConnection returns rows for any term, so real-DB recall is not exercised. Consistent with ADR 0150's stated design.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| def row_matches_time_range( | ||
| row: Any, | ||
| time_range: tuple[date, date] | None, | ||
| ) -> bool: | ||
| """Keep rows inside the Seoul window, or keep them when clocks are absent.""" | ||
| if time_range is None: | ||
| return True | ||
| instant = ask_filter_instant(row) | ||
| day = seoul_calendar_date(instant) | ||
| if day is None: | ||
| return True | ||
| start, end = time_range | ||
| return start <= day <= end |
There was a problem hiding this comment.
📝 Info: Overflow window excludes all rows and adds no axis fact
resolve_korean_relative_time returns _EMPTY_DATE_RANGE (date.max, date.min) on overflow (temporal_expressions.py). That is non-None, so time_filter_active becomes True and both the SQL bound and row_matches_time_range drop every row, yielding no sources and no time-axis fact. 언젠가 still returns None. Pre-existing, unaffected here.
Was this helpful? React with 👍 or 👎 to provide feedback.
Main's 0183 slot is taken by the GNB four-Korean-chrome ADR (#587), so the incoming ask-event-time-filter ADR moves to the next free number (0202). All references across AGENTS.md/CLAUDE.md/ARCHITECTURE.md, docstrings, ADR 0150 related links, seed comments, and changelog fragments follow. CHANGELOG.md keeps both the 2.13.0 and 2.15.0 entries in descending order.
| `report_leftover_map_coverage` and captions the pair list with how | ||
| many scored posts entered the map. | ||
|
|
||
| Global Ask relative-time filters (ADR 0150 / 0183) bind to |
There was a problem hiding this comment.
🟡 Event-time feature cites unrelated ADR number
The added rule cites ADR 0150 / 0183, but 0183 is the GNB-chrome ADR (docs/adr/0183-gnb-four-korean-chrome.md); the event-time decision is ADR 0202. 0183_source_post_event_occurred_at.sql repeats the same wrong citation. Readers are sent to an unrelated ADR.
| Global Ask relative-time filters (ADR 0150 / 0183) bind to | |
| Global Ask relative-time filters (ADR 0150 / 0202) bind to |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Fixes #569. Global Ask relative-time questions (
어제,그저께,지난주) now bind to the source-system event instant, not record ingestion time (ADR 0168).Bulk imports cluster
created_atnear import time, so어제 무슨 일이 있었나요?used to miss yesterday's events. Ask now comparescoalesce(event_occurred_at, created_at)on the Asia/Seoul calendar. Missing event time falls back tocreated_atand is named as that fallback — never invented.After
make seed, leftover closest/farthest pairs still sit above the member list; a click still opens that post. Ask 어제 무슨 일이 있었나요? on bulk-imported fixtures that share one ingest day keeps the post whose event fell yesterday, drops last week's event, and names Time axis on the cited evidence. Open that cited post to read which clock matched.Decision (ADR 0168)
source_post.event_occurred_at(Allen 1983; W3C Time; PROV-Oprov:generatedAtTimestays oncreated_at).time axis: event occurred atortime axis: record created at.--event-occurred-at-column. Unmapped imports leave the column null.occurred_atonto both clocks so reconstruct and leftover pairs stay on the designed January timeline.created_at.Tests
uv run --python 3.12 --extra dev --with asyncpg pytest tests/test_ask_time_axis.py tests/test_global_ask_sources.py tests/test_post_chat.py tests/test_import_postgresql_posts.py tests/test_temporal_expressions.py— 90 passed, 2 skipped.pnpm exec vitest run src/components/AskEvidenceLayerPopup.test.tsx— 9 passed.Synthetic fixtures only. Independent APPROVE required. Do not self-approve. Squash-merge only with independent APPROVE + exact-head required success.