Skip to content

Sort by game_id and created_at for events, use cursor in export#922

Merged
tudddorrr merged 1 commit intodevelopfrom
events-sort-key
Apr 8, 2026
Merged

Sort by game_id and created_at for events, use cursor in export#922
tudddorrr merged 1 commit intodevelopfrom
events-sort-key

Conversation

@tudddorrr
Copy link
Copy Markdown
Collaborator

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Apr 8, 2026
@tudddorrr tudddorrr changed the title Sort by game_id and created_at first for events, use cursor in export Sort by game_id and created_at for events, use cursor in export Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.59%. Comparing base (b40d97f) to head (e2a8146).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #922      +/-   ##
===========================================
- Coverage    97.60%   97.59%   -0.02%     
===========================================
  Files          406      406              
  Lines         6648     6652       +4     
  Branches       881      882       +1     
===========================================
+ Hits          6489     6492       +3     
- Misses          75       76       +1     
  Partials        84       84              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 8, 2026

PR Review: Sort by game_id and created_at for events, use cursor in export

The core approach is solid — cursor-based pagination is the right fix for large event datasets where OFFSET becomes increasingly expensive, and reordering the sort key to (game_id, created_at, id) makes the new query pattern efficient. One minor thing worth noting.


5. Test Coverage

🔵 No test for multi-page cursor iteration

The cursor-update code path (lines 177–179 of dataExportProcessor.ts) is exercised only when a full page of 10,000 events is returned and another page must be fetched:

const last = rawEvents.at(-1)!
lastCreatedAt = new Date(last.created_at)
lastId = last.id

Codecov confirms this as 1 new uncovered line. The tuple cursor comparison (e.created_at, e.id) > (toDateTime64(...), lastId) is the core correctness concern of the PR and isn't exercised anywhere in the test suite. Worth adding a test that returns exactly PAGE_SIZE rows on the first call and a smaller set on the second to confirm the cursor is built and applied correctly.


Everything else looks good

  • Migration is well-structured: Starting with DROP TABLE IF EXISTS events_new before CREATE TABLE events_new correctly handles partial-failure retries — if the migration dies mid-flight, the next run cleans up the staging table and restarts cleanly. The atomic EXCHANGE TABLES is the right choice over RENAME TABLE (used in migration 004) as it leaves no window where events is unavailable.
  • Sort key and cursor match: The new sort key (game_id, created_at, id) aligns with both the WHERE e.game_id = X filter and the ORDER BY e.created_at ASC, e.id ASC clause, so ClickHouse can seek directly to the cursor position within a game's partition.
  • Cursor placement: Moving the cursor update to after the break check is correct — no need to update state on the final (partial) page.

@tudddorrr tudddorrr merged commit a45bb51 into develop Apr 8, 2026
9 of 10 checks passed
@tudddorrr tudddorrr deleted the events-sort-key branch April 8, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix This pull request contains a fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant