Use simple queries (no joins or groups) for fetching events and props

[tudddorrr]

No description provided.

[claude]

PR Review: Use simple queries for fetching events and props

This is a clean refactor that replaces a complex dynamic pivot query with two straightforward queries. The approach is well-motivated and the implementation is solid.

---

1. Code Quality and Best Practices

No issues found. The assert(eventProps) after the has() check is the right tool for satisfying TypeScript narrowing, and removing the as number / as string casts in the hydration block is a good cleanup.

---

2. Potential Bugs

🔵 Minor — Props written concurrently during a long export may be silently dropped

propKeys is fetched once (step 1) before pagination begins. Hydration filters exclusively through that snapshot:

event.props = propKeys
  .filter((key) => eventProps[key] != null)
  .map((key) => ({ key, value: eventProps[key] }))

Any prop key that appears in rawProps but was not in the initial propKeys snapshot (e.g., written concurrently during a long export) will be silently dropped. This is the same behaviour as the old pivot approach — not a regression — but worth noting given the page size has grown 10×, which extends the window.

---

3. Performance Considerations

🔵 Minor — Events table is scanned twice per page

The props subquery re-runs the same cursor-filtered scan on events. The comment explains this avoids passing up to 10,000 IDs as HTTP params, which is a reasonable trade-off. With a proper MergeTree ordering key on game_id this should be cheap in practice.

---

4. Security Concerns

No issues found. game.id is an integer, lastId is a UUID (hex + hyphens only), and formatDateForClickHouse sanitises the timestamp — none of these accept free-form user input.

---

5. Test Coverage

The PR adds a solid pagination test that verifies both the count (20,002 events across three pages) and that props are correctly hydrated on every event. That covers the core logic changed here.

Two minor branches remain uncovered (flagged by Codecov):

• The includeDevData = false path — the new test always passes true, so the AND dev_build = false clause in both queries is never exercised.
• The rawEvents.length === 0 branch of the rawProps ternary — only reachable when the event count is an exact multiple of PAGE_SIZE, which 20,002 is not.

Neither is blocking, but a test with includeDevData = false would be cheap to add and would catch any accidental regression in the filter string.

---

Overall: The refactor is well-motivated, cleanly implemented, and now has meaningful test coverage. The items above are all minor observations rather than blockers.

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.65%. Comparing base (2b81f8f) to head (1cabfec).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/lib/queues/data-exports/dataExportProcessor.ts	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #926      +/-   ##
===========================================
+ Coverage    97.59%   97.65%   +0.06%     
===========================================
  Files          406      406              
  Lines         6652     6656       +4     
  Branches       882      885       +3     
===========================================
+ Hits          6492     6500       +8     
+ Misses          77       72       -5     
- Partials        83       84       +1     

☔ 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.