[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:
  • SYSTEM REFRESH VIEW;
  • SYSTEM WAIT VIEW, so a broken refresh query makes dbt run fail instead of failing quietly;
  • a full-history insert (toDateTime(0)), which picks up events older than the window, such as
    late or replayed ones. It runs in 16 learner chunks (ASPECTS_VIDEO_WATCH_INTERVALS_BACKFILL_CHUNKS)
    to cap memory; each chunk reads all of video_playback_events.
• 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.
• emission_time_long keeps sub-second precision (DateTime64(6)). It used to be stored as whole
  seconds, so events in the same second were ordered by verb and event_id instead of time. A
  post_hook converts the column on existing installs; rows already stored keep their truncated
  times until video_playback_events is rebuilt with --full-refresh.

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 going through the per-second rows of fact_video_segments.
• 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 the model is a view, a leftover MV would still try to write into it and 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 of operation.

---

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. The inherited off-by-one at interval starts is fixed in the last commit (backported from #180)

After chunking, the 44.79M event run did not fail:

dbt run -s video_playback_events+ --full-refresh: PASS, 3 m 54 s. The video_playback_events
rebuild took 109 s (655 MiB). The intervals back-fill took 16 chunks of 6.6–8.1 s, 113 s in total,
peak 3.25 GiB. Unchunked, the same back-fill fails with MEMORY_LIMIT_EXCEEDED at ~13.4 GiB.

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.

[felipemontoya]

Thank you for all the great follow ups @bmtcril.

I had my environment running a bunch of tests yesterday and I think I can answer your questions now.
Btw, for the tests, I did take 2 commits from #180 and put them in this branch. They made sense to me in the same context, but if I push them your PR 180 will need a rebase and I can understand if you prefer them in there.

• b1bedd3 fix: count video seconds from the start position and tolerate open intervals
  An interval from 10 to 20 covers seconds 11 to 20, not 10 to 20. ifNull stops the
  "Number of Views across Video Duration" chart failing on open intervals.

• 70c7e1a fix: keep sub-second precision in video_playback_events.emission_time_long
  emission_time_long resolved to the DateTime alias and was stored as whole seconds, so
  events within the same second paired by verb and event id instead of time. A post_hook
  converts the column on existing installs; rows already stored keep their truncated times.

Now on to the test runs:

Setup

• ClickHouse 25.8. The ClickHouse container was capped at 4 CPU / 16 GB for every run.
• Three datasets, each generated with its expected per-learner results:
  • oracle: 9K events.
  • m: 14.8M events, 6.3M of them video.
  • A scaled-up l loaded on top of m: 89.3M more events. That brings video_playback_events to 44.74M rows spread over 12 months, the size of the failing case in the description.

Correctness first

Video seconds per learner and video, compared with the known answers:

	main	branch, upgraded without --full-refresh	branch, rebuilt
oracle (768 learner-videos)	520 exact	761	768 (100 %)
m (1,291,131 learner-videos)	883,553 (68.4 %)	1,282,767 (99.35 %)	1,291,131 (100 %)
m + l at 44.74M (9,116,749 learner-videos)	–	–	9,116,749 (100 %)

• The commit before the backport (efb9e5c) got 520 of the 768 oracle learner-videos exact.
  • 242 of the 248 wrong ones were the off-by-one: one extra second per interval that doesn't start at 0.
  • The other 6 were the pause and resume in the same instant case.
• c4734e6 fixes both.

1. Upgrade with substantial existing data

• On m (14.8M events, loaded under main), the upgrade dbt run takes 46.6 s, PASS=37, no errors.
• Without --full-refresh the result is already 99.35 % exact. The rest needs a rebuild of video_playback_events, because rows stored under main keep whole-second emission_time_long values.
• At 44.74M I timed the steps that grow with the data:
  • The modify column emission_time_long DateTime64(6) post_hook:
    • It returns in 0.6 s and runs as a background mutation, since dbt doesn't set mutations_sync.
    • The mutation finished 1.1 s after the ALTER was sent and used 7 MiB.
    • The table stays readable throughout.
  • The first back-fill of fact_video_watch_intervals: 113 s.
  • The rest is dropping the two old MVs and creating two views.
• So an upgrade at 44.74M should take about 2–3 minutes. That figure is a sum of the steps, not a single upgrade run at that size.

2. Per-refresh cost, partition pruning, minmax index

• The refresh only reads the current month partition. Within that month nothing is pruned, because emission_time is the 4th column of the sort key, after (org, course_key, verb_id).

  • On m: 136K rows read (18K inside the 1-day window), 38–52 ms, 16 MiB.
  • At 44.74M: 932K rows read (124K inside the window), 0.1–0.2 s, 77 MiB.

• A minmax index on emission_time (granularity 1) changed nothing on m. Each granule spans most of the month, so min/max can't exclude it.

• Worst case, a synthetic busy month with 31.7M rows in one partition (2.43M inside the window):

  variant	rows read	time
  as is	25.9M	3.4–4.1 s
  + minmax index	10.3M	3.4–3.5 s
  daily partitions	4.7M	3.2–3.4 s
  projection ordered by time	2.43M	5.0–5.8 s
  only the window's rows (lower bound)	2.59M	2.5–2.7 s

  The month scan costs at most about 1 s per refresh, against a 5-minute schedule. I dont think we need to change the partitioning or add an index.

• Scheduled refreshes kept running during the 89M-event insert at 0.1–0.4 s each, with one at 3 s.

3. The 44.8M case with chunking

At 44.74M video events, under the 16 GB cap:

run	result	time	peak memory
dbt run -s video_playback_events+ --full-refresh	PASS=4	3 m 54 s total: rebuild 109 s, back-fill 16 chunks 113 s	3.25 GiB
dbt run -s fact_video_watch_intervals (the back-fill every run does)	PASS=1	2 m 06 s	3.23 GiB

Chunk count sweep, same SQL:

chunks	time	peak memory
1	fails: MEMORY_LIMIT_EXCEEDED at 13.4 GiB	–
2	52 s	8.5 GiB
4	62 s	5.8 GiB
8	82 s	4.0 GiB
16 (default)	113 s	3.25 GiB

So unchunked it reproduces the failure in the description, and the default of 16 chunks passes with about 4× headroom.

After this tests, I got a suggestion from claude to use FINAL instead of limit 1 by event_id. I'm running a tests to determine if it works as the agent suggests.

Charts

Same course and learner, with course-staff row-level security, warm runs:

chart	m	44.74M	44.74M + tutor-contrib-aspects#1355
Number of Views across Video Duration	34–35 s	40–41 s	12.5–13 s
Partial and Full Video Views, Video Engagement	38–45 s	44–49 s	needs fact_video_watches from #180
Video engagement per section / subsection	1.0–1.5 s	3.0–3.5 s	2.9–3.5 s

• The slow video_watches charts cost about the same for one course whatever the table size, which fits your diagnosis that the Superset dataset is the cause.
• #1355's watched_video_segments change works on this branch as is. Its video_watches.sql reads fact_video_watches, which only exists with perf: fix engagement chart numbers and improve video chart performance #180, so I couldn't measure those charts here.
• The per-section / per-subsection charts read rows in proportion to the whole intervals table (10.2M rows read for one course). That may be worth a look separately.

---

I finished running the test that was suggested for the chunking and if that goes well I'll update the PR in a while.
I'm fixing the description as well.

[felipemontoya]

Using final did not work. I'm keeping the current limit 1 by event_id.

• FINAL without chunks: memory grows with the largest course, about 0.18 GiB per million rows above roughly 5M. On a 16 GB server it would fail somewhere between 20M and 44.7M rows in one course. Real courses are far smaller (181K here), but it is still a ceiling that chunking does not have.
• Either variant with 16 chunks: about 1–1.5 GiB at every course size, because learner chunks split even a giant course.
• Speed: the current limit 1 by chunks are 2.5–3.5× faster per chunk than FINAL chunks. Sixteen of them take about 50–80 s, against 3–5 min for FINAL chunks.

---

After I updated the PR description, I decided that keeping the performance fixes in this PR was the best path. So I'm pushing the last commit from my local branch with your fixes.

[bmtcril]

Thanks that all makes sense! Just a thing to note- The per-section / subsection charts reading 10.2M rows for one course is already fixed in #180 (0de2c67, which lets the course filter reach every read; 0.55 s → 0.28 s on M). So you shouldn't need to do anything there. I'll merge this and rebase 180 on it.