Video segment pairing fix

[felipemontoya]

This PR contains the fix to issue #176. It has three commits: the fix, the tests and the performance changes.

The problem it fixes

fact_video_segments used to be a plain ClickHouse materialized view (MV) over video_playback_events. A plain MV only sees the rows in each insert batch. If a learner's played event and its closing paused event arrive in different inserts, which is normal for live traffic, they are never paired. That watched time is silently lost. fact_video_engagement was an MV built on top of it, so it lost the same data.

---

The new design

1. New model: models/video/fact_video_watch_intervals.sql

• It is a refreshable materialized view (needs ClickHouse 24.10 or later). It re-runs on a timer, every 5 minutes with up to 1 minute of random jitter by default. In append mode it writes into a ReplacingMergeTree(computed_at) table.
• Each refresh only looks at a recent window of events, 1 day by default, and pairs them again. Because the table keeps the newest row per start_event_id, an interval that was open on one refresh is replaced once its closing event shows up.
• The window and the timing can be changed with three environment variables: ASPECTS_VIDEO_WATCH_INTERVALS_LOOKBACK, _REFRESH and _REFRESH_RANDOMIZE.
• The post_hook runs three steps on every dbt run:
  • a full-history insert (toDateTime(0)), which picks up events older than the window, such as late or replayed ones;
  • SYSTEM REFRESH VIEW;
  • SYSTEM WAIT VIEW, so a broken refresh query makes dbt run fail instead of failing quietly.
• catchup=False stops the refresh from also back-filling history, since the post_hook already does that.

2. The pairing logic: macros/video_watch_intervals.sql

• Input: it reads video_playback_events, drops initialized events, and keeps one copy per event_id (limit 1 by event_id). Without that, an unmerged duplicate could close a play with its own copy.
• Pairing: it groups events by (org, course, actor, video). For each event it uses leadInFrame to find the next event in that group.
• Ordering: events are sorted by time. When timestamps are equal, a closing event goes before a played, and event_id breaks any remaining tie, so every refresh produces the same result.
• Output: one row per played event, with:
  • start and end position;
  • end_verb_id;
  • is_interval_closed (a next event exists);
  • is_watched (closed, and the position moved forward);
  • computed_at, which is a macro so tests can fix its value.
• Change in behaviour: any following event now closes a play: paused, seeked, completed, terminated, or another played. The old code paired each start with the last end event before the next start.

3. fact_video_segments is now a view

• It reads the watched intervals (with FINAL) and expands each one into one row per second with arrayJoin(range(...)).
• Breaking change: watch_count is now always 1. A second watched twice shows up as two rows, so it has to be summed. Before, the model grouped rows and stored a count per second.

4. fact_video_engagement is now a view

• It reads fact_video_watch_intervals where is_watched directly instead of goi
• It deliberately skips FINAL. The code comment accepts that, rarely, an old row that has not yet been merged away counts one extra video.

5. Cleaning up the old objects

• Both converted models have a pre_hook that drops the old *_mv view. Once theover MV would make xAPI inserts fail.
• remove_deprecated_models also drops those old views.
• The README gets a breaking-change note and schema.yml documents the new mode

---

How it is tested

1. dbt unit tests (models/video/unit_tests.yaml)

• test_fact_video_watch_intervals (new)
• test_fact_video_segments (fixed)
• test_fact_video_engagement (fixed)

2. CI integration check (.github/workflows/coverage.yml)

• This is the regression test for the actual bug, which a unit test cannot reproduce because unit tests run as a single query.
• After dbt run and dbt test, it uses curl to insert two raw xAPI events for a new learner into xapi.xapi_events_all, as two separate inserts: a played at position 0 and a paused at position 40.
• It forces a refresh of fact_video_watch_intervals_mv and waits for it.
• It then checks for exactly one watched interval from 0 to 40 in fact_video_watch_intervals FINAL, and exactly 40 rows in fact_video_segments.
• If either check fails, it prints the intervals and system.view_refreshes, then fails the job.

3. Performance:
In a local tutor environment. The host: 12 CPUs, 30 GiB RAM, of which ~11 GiB were free when testing started (the rest
held by the desktop and other projects), NVMe disk. ClickHouse 25.8.33.6 in
tutor_dev-clickhouse-1, no container limits, max_server_memory_usage 27.9 GiB (0.9 × RAM),
max_threads 12, no per-query max_memory_usage. The host has less free memory than
ClickHouse thinks it may use, so host memory pressure can be the real limit.

Incremental: the refresh keeps up far beyond realistic traffic; memory is the limit, not time.
One refresh re-pairs the last day of video events:

video events in the 1-day window	refresh	memory (query)	container peak
0.44M	0.3 s	0.2 GiB	2.5 GiB
4.37M	4.5 s	2.15 GiB	4.5 GiB
8.19M	5.9 s	2.93 GiB	5.3 GiB
15.84M	15.6 s	4.77 GiB	7.5 GiB
31.12M	36.5 s	8.49 GiB	11.4 GiB

At 31M video events per day (~70M xAPI events per day at the generator's 44% video share) a refresh
takes 37 s against a 5-minute interval. The next doubling would need more memory than this host has
free. Ingestion itself tops out at ~13k xAPI events/s (~1.1B/day) through the full Aspects MV chain,
far above both.

Full refresh: 10–28× faster than v8.0.0 and 4–10× less memory, but it does not stay small.
Like-for-like on the same course subsets (runs 25–27):

video events	branch: full-history insert	v8.0.0: back-fill	watched seconds, v8 vs branch
3.26M	4.1 s, 1.76 GiB	121 s, 17.4 GiB	+13%
7.23M	8.6 s, 3.09 GiB	261 s, 17.7 GiB	+13%
14.02M	17.9 s, 4.73 GiB	491 s, 20.2 GiB	+13%
44.79M	40 s, 13.0 GiB alone; fails inside dbt run --full-refresh	fails (killed at 25 GiB after 80 s, nothing written)

v8.0.0's extra 13% of watched seconds is its pairing defect (the last end before the next play).
Both still carry the inherited off-by-one at interval starts.

This performance review raised 5 findings, fixed in the last commit.
A more complete depiction of all the tests run was created using claude artifacts and its available here: https://claude.ai/artifact/Gctp5pViYWeFCh2cV2CtWn

[openedx-webhooks]

Thanks for the pull request, @felipemontoya!

This repository is currently maintained by @openedx/committers-analytics.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[felipemontoya]

@saraburns1 rebased the PR. I also included all your suggestions and took it a step further trimming comments to mostly one line at the top of the file or comments that are necessary to understand the intent of confusing lines.

[bmtcril]

This is great overall! Because this is a bit of a new direction, and if it works well is probably something we would want to carry over to pages and problems, I have a couple of questions:

1. Have you tested against an upgrade of substantial existing data from an older version of aspects-dbt? I don't expect any issues, but sometimes dbt throws us curve balls changing materializations.
2. Can you get performance numbers on individual refreshes? video_playback_events is partitioned by month. The "last 1 day" filter can probably only skip whole months, so each 5-minute refresh may read the whole current month. The benchmark doesn't say how your data was spread over time. So if you didn't, can you try with data spread out over a few months and see if that changes things? A minmax skip index on emission_time would likely fix it either way.
3. Have you re-run the 44.8M-event full-history case with chunking? The description shows it failing, but the chunking commit came afterwards, and this backfill runs on every dbt run, not just full refreshes.
4. The PR description lists the post_hook order as backfill -> refresh -> wait, but the code does refresh -> wait -> backfill. A few sentences are also cut off ("instead of goi", "Once theover MV").

[bmtcril]

I've been benchmarking the engagement models against a dataset with known expected results (~14.8M xAPI events, 25k learners, realistic play/pause/seek journeys) on ClickHouse 25.8 limited to 4 CPU / 16 GB. I added a round for this PR before starting on other performance improvements, and here's what I found.

I rendered the actual Superset chart SQL from tutor-contrib-aspects for the Course Dashboard and Individual Learner video tabs, then ran it directly.

Good findings:

• Pairing is correct: watched seconds come out within +0.1% of expected. The remainder is the inherited off-by-one you mention.
• The full-history insert is fast: 5.7 s for 6.3M video events. v8.0.0's back-fill on the same data needed 16.4 GiB and ran out of memory.
• Inserts no longer fail. v8.0.0 could not take this dataset as one bulk insert.

Issues found:

1. Bug: "Number of Views across Video Duration" fails

Code: 349. Cannot convert NULL value to non-Nullable type: while executing
'FUNCTION CAST(end_video_position :: 1, 'int'_String :: 2)'

The watched_video_segments dataset wraps the view in a with and an OR course filter. That lets ClickHouse evaluate the cast before where is_watched removes the open intervals. Fix (verified):

cast(ifNull(end_video_position, 0) as int) + 1,

2. The video_watches charts are 2–3× slower than v8.0.0

Largest course (~1,050 learners), warm runs:

Chart	v8.0.0	this PR
Partial and Full Video Views / Video Engagement, course staff	8.5 s	20 s
Same charts, admin (no RLS)	24 s	68–75 s

Peak memory is about 7 GiB per chart on both. Loading the whole Videos tab at once still runs out of memory on 16 GB with either version.

Since fact_video_segments is now a view, the per-second rows (16–26M for this course) are built on every chart query instead of read from storage. Suggestion: materialize the per-second (or aggregated) segments from fact_video_watch_intervals, so the charts read stored rows again. This could be a second refreshable MV chained with depends_on. An alternative is a per-(learner, video) summary with distinct seconds and max watch count, which is all video_watches needs. I haven't benchmarked either option yet. Most of the remaining cost is in the Superset dataset, which groups per-second rows by ~16 string columns, so that fix belongs in tutor-contrib-aspects.

3. Note on the +13% figure

On our dataset, v8.0.0's watched seconds come out only +0.4% above expected, not +13%. The "last end before the next play" defect only shows up on interleaved event sequences, which the random load generator produces much more often than real journeys do. The fix is still right, but real-world drift is probably less than 13%.

I've been using this xapi-db-load branch to test large scale data that's not random, and yields much more realistic results: openedx/xapi-db-load#276

[felipemontoya]

Fantastic review. I am a bit behind on my side, but I expect to get back on track on Tuesday.

[bmtcril]

A correction and two more findings, now that we've profiled further.

Correction: the view isn't what makes the video charts slow

In my last comment I put the 2–3× slowdown of the video_watches charts down to fact_video_segments becoming a view. Profiling says otherwise. The cost is the Superset dataset itself, which groups one row per watched second (about 30M for a 1,000-learner course) by about 16 columns. Here is the same chart query on the same data with three different sources:

fact_video_segments source	Video Engagement chart
this PR's view	25 s
the same rows stored in a table	18 s
stored, pre-grouped per learner/video/second	17 s

v8.0.0 looked faster mostly because it stored fewer rows. So there's no need to materialize fact_video_segments in this PR. Please disregard that suggestion.

What does fix it is a per-learner, per-video summary: distinct seconds watched, plus the most views of any second. That's all the video_watches dataset needs. Built from fact_video_watch_intervals and refreshed right after it, it takes those charts from about 20 s to about 60 ms, matching the per-second counts exactly. Since that needs a tutor-contrib-aspects dataset change too, which I can raise in a PR there.

Finding: emission_time_long has no sub-second precision, which affects the pairing here

In video_playback_events, emission_time as emission_time_long resolves to the CAST(emission_time, 'DateTime') as emission_time alias in the same select. So emission_time_long is stored as a whole-second DateTime:

select type from system.columns
where table = 'video_playback_events' and name = 'emission_time_long'
-- DateTime

The pairing window here orders by emission_time_long, so events within the same second fall back to the verb/event_id tie-break rather than their real order. Fix: compute toDateTime64(emission_time, 6) in the CTE and select that. Existing tables also need alter table ... modify column emission_time_long DateTime64(6), since dbt only updates the MV query on existing installs. The bug predates this PR, so it could equally be fixed separately.

Finding: the off-by-one is easy to fix here

The off-by-one you noted is range(greatest(start, 1), end + 1). If second n means playback from n − 1 to n, an interval from 10 to 20 covers seconds 11–20, but this counts 10–20. range(start + 1, end + 1) fixes it, and it gives the same result as today for intervals starting at 0. Combined with the ifNull fix from my last comment:

range(
    cast(start_video_position as int) + 1,
    cast(ifNull(end_video_position, 0) as int) + 1,
    1
)

With both fixes, watched seconds match a known-answer dataset exactly (14.8M events, 25k learners).

[bmtcril]

Last thing, then I'll stop spamming this ticket. In trying to figure out the tutor-contrib-aspects bug 1335 I built on top of this PR to get to #180 and openedx/tutor-contrib-aspects#1355

I think they fix the items I brought up above and get us to a much better overall place with both data loading and engagement chart performance, but would really appreciate your input and review.