[codex] Fix ClickHouse activity dedupe build - #1218
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR refactors the activity deduplication pipeline from a recursive-macro approach ( ChangesActivity Deduplication Pipeline Refactoring
Sequence DiagramsequenceDiagram
participant postgres_fitness.activity as Activity Source
participant activity_source_records as Source Records
participant activity_duplicate_matches as Duplicate Matches
participant activity_duplicate_groups as Duplicate Groups
participant deduped_activities as Deduped Activities
Activity Source->>Source Records: filter active, compute priority
Source Records->>Duplicate Matches: normalize ended_at, identify overlaps
Duplicate Matches->>Duplicate Groups: expand connectivity, assign group_id
Duplicate Groups->>Deduped Activities: join with source records
Deduped Activities->>Deduped Activities: select canonical by priority, aggregate fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Storybook previews for This comment updates automatically on each PR push. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Automated Checks (advisory, non-blocking)✅ All checks passed. Surmado Code Review — Free tier limit reachedYou've used all 10 free reviews this month. Deterministic checks (secrets, model strings) still ran above. Upgrade to the Paid plan for 100 reviews/month + $15 per additional 100: https://app.surmado.com/checkout?plan=pr_review_starter Or wait until your next monthly window for 10 more free reviews. Surmado Code Review (v1.2-mt) |
|
Review app deployment was skipped for PR #1218. Hetzner could not allocate the configured review app server type in the configured location. This is provider capacity/placement availability, not a code failure in this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@analytics/models/read_models/activity_duplicate_groups.sql`:
- Around line 33-75: The current
duplicate_walk_rows/duplicate_walk/current_duplicate_groups logic only explores
up to two hops and must be replaced with a transitive-closure approach: stop
using the fixed first_link/second_link joins and instead iteratively expand
connected pairs from duplicate_links and source_records until no new
(activity_id, connected_activity_id) pairs are produced (or use a recursive CTE
pattern supported by our SQL engine), then compute group_id as the min
stringified connected_activity_id per activity from that converged closure;
reference duplicate_links, source_records,
duplicate_walk_rows/duplicate_walk/current_duplicate_groups when locating the
affected code and ensure the iteration converges (idempotent) so all chain
lengths like A-B-C-D collapse to one group.
In `@analytics/models/read_models/activity_source_records.sql`:
- Around line 38-40: The row_number() selection for device-priority is
non-deterministic when two active_device_priority rows have equal
source_name_pattern length; update the ORDER BY inside row_number() OVER
(PARTITION BY active_activity.id ORDER BY
length(active_device_priority.source_name_pattern) DESC, ...) to add a total
tie-breaker such as active_device_priority.id (or another unique/stable column)
so ties are resolved deterministically and the derived priority/canonical
activity in deduped_activities.sql remains stable.
In `@analytics/models/read_models/read_model_microbatch.sql.test.ts`:
- Around line 96-98: Remove the negative assertions that check for the deleted
dedupe implementation in the test (the expect(...).not.toContain checks for "{{
activity_dedup_graph() }}", "connected_components AS", and
"visited_activity_ids") from
analytics/models/read_models/read_model_microbatch.sql.test.ts (also remove the
duplicate checks referenced at the other ranges); instead keep and/or strengthen
the positive assertions that validate the new staged models and expected SQL
fragments for the current implementation (look for test helpers or existing
positive expect(sql).toContain(...) checks that assert the new staged model
behavior and leave those intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 775519ed-d0bb-4c25-a546-9ab83a1bcd04
📒 Files selected for processing (8)
analytics/macros/bounded_activity_graph.sqlanalytics/models/read_models/activity_duplicate_groups.sqlanalytics/models/read_models/activity_duplicate_matches.sqlanalytics/models/read_models/activity_source_records.sqlanalytics/models/read_models/deduped_activities.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsdocs/production-incident-baseline.mdentrypoint.sh
💤 Files with no reviewable changes (1)
- analytics/macros/bounded_activity_graph.sql
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR significantly restructures the core activity deduplication logic, replacing a recursive CTE with multiple incremental read models, which is a high-risk change to production data pipelines that requires human review to ensure correctness and data integrity.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 8 files
Confidence score: 3/5
- There is concrete data-quality risk in
analytics/models/read_models/activity_duplicate_groups.sql: limiting duplicate grouping to 2 hops can split a single connected duplicate chain into multiple groups, which may produce multiple canonicals for one real activity. analytics/models/read_models/deduped_activities.sqlhas a likely regression risk because stale-match filtering only usesactivity_idwhile dedupe identity is(user_id, activity_id), so outdated rows can persist whenuser_idchanges.- This lands at moderate merge risk (not an immediate blocker) because the issues are medium-high severity with high confidence and affect correctness/determinism in read models rather than just style or housekeeping.
- Pay close attention to
analytics/models/read_models/activity_duplicate_groups.sql,analytics/models/read_models/deduped_activities.sql, andanalytics/models/read_models/activity_source_records.sql- fix connected-component grouping depth, key matching on both dedupe columns, and deterministic tie-breaking in ranking.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR fundamentally rewrites the activity deduplication pipeline, replacing a recursive CTE with four new incremental models, which is a high-impact change that touches core business logic and data infrastructure, and a CI issue was discovered after production deployment, so human review is...
Re-trigger cubic
Replaces the failing recursive ClickHouse activity dedupe graph with domain read models for source records, duplicate matches, duplicate groups, and deduped activities.
This fixes the production activities empty state caused by
analytics.deduped_activitiesstaying empty after ClickHouse hitMEMORY_LIMIT_EXCEEDEDin the old recursive model.Production has already been deployed to
sha-142216b; the first analytics-worker dbt build completed successfully with 15/15 models in 28.24s and ClickHouse now reports 73 recent deduped activities.Validated with focused read-model tests, analytics policy lint, root/server/web typechecks, production health check, and production ClickHouse row-count checks.
Summary by cubic
Rebuilt ClickHouse activity dedupe as incremental read models so builds are memory-safe and
analytics.deduped_activitiesis repopulated. Adds deterministic matching and fixes a nullable sort key that blocked clean first builds and CI.Bug Fixes
activity_source_records→activity_duplicate_matches→activity_duplicate_groups→deduped_activities(allReplacingMergeTree(refresh_version)), eliminating ClickHouseMEMORY_LIMIT_EXCEEDED; dbt build succeeds and recent activities now show in production.deduped_activitiesby emittingassumeNotNull(user_id), fixing the ClickHouse “allow_nullable_key” error in clean e2e builds.user_idsort key; added the new models toDBT_SAFE_MODELSinentrypoint.sh.Refactors
analytics/macros/bounded_activity_graph.sqland rewireddeduped_activitiesto use the new read models.max_threads: 1,join_use_nulls: 1) and deterministic device-priority tie-breakers (pattern length DESC, priority ASC, pattern ASC).Written for commit ac5ee70. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor