Speed up player event fetching

[tudddorrr]

No description provided.

[claude]

Code Review: Speed up player event fetching

✅ Performance Improvements

This PR makes an excellent performance improvement by eliminating SQL injection risk and string concatenation overhead:

• Before: String concatenation of alias IDs into the query (WHERE player_alias_id IN (${aliases}))
• After: Using parameterized query with array parameter (WHERE player_alias_id IN ({aliasIds:Array(UInt32)}))

This change provides:

• Better performance: Removes string manipulation overhead
• Query plan stability: ClickHouse can better optimize queries with typed parameters
• Type safety: Explicitly declares UInt32 array type

✅ Security

The parameterized approach is more secure as it prevents any potential injection attacks through alias IDs, even though they're internally generated.

✅ Code Quality

• Clean, minimal change that follows DRY principles
• Maintains existing functionality while improving performance
• Type annotations are appropriate (Array(UInt32))

✅ Test Coverage

Excellent test coverage exists in tests/services/player/events.test.ts covering:

• ✅ Basic event retrieval
• ✅ Search functionality
• ✅ Pagination
• ✅ Access control
• ✅ Error handling

🔍 Minor Considerations

1. Edge Case: Consider adding validation for empty alias arrays, though this should be rare in practice since players always have at least one alias.

2. Query Consistency: The change maintains consistency with ClickHouse best practices for array parameters.

Verdict: ✅ APPROVE

This is a well-implemented performance optimization with proper parameterization. The change is minimal, focused, and maintains backward compatibility while improving query performance.

Ready to merge! 🚀

[claude]

Code Review: Speed up player event fetching

✅ Performance Improvements

This PR successfully addresses performance issues with the player events query by:

1. Eliminating unnecessary LEFT JOIN: The removal of LEFT JOIN event_props and DISTINCT significantly reduces query complexity and execution time
2. Adding targeted index: The new event_id_idx minmax index on event_props.event_id will improve performance for the EXISTS subquery
3. Using parameterized queries: Converting from string concatenation to {aliasIds:Array(UInt32)} is both safer and more efficient

🔧 Code Quality

Good practices:

• Proper migration structure following existing patterns in src/migrations/clickhouse/index.ts:43-44
• Consistent naming convention for the migration
• Using EXISTS subquery for search functionality is appropriate

Areas for consideration:

• The migration does not include a rollback/down migration, though this appears consistent with the codebase pattern
• Consider adding a comment explaining the performance reasoning in src/services/player.service.ts:315-316

🛡️ Security

✅ Excellent improvement: Replacing string concatenation (player_alias_id IN (${aliases})) with parameterized queries ({aliasIds:Array(UInt32)}) eliminates potential SQL injection vulnerabilities.

🐛 Potential Issues

1. Search behavior change: The old query searched across all event properties via JOIN, while the new query uses EXISTS. This could have different performance characteristics for large result sets, but should produce the same results.

2. Index granularity: GRANULARITY 64 is reasonable for most use cases, but consider monitoring query performance to ensure this value is optimal for your data distribution.

📊 Test Coverage

Missing: No tests found specifically for the events endpoint in PlayerService. Consider adding integration tests to verify:

• Search functionality works correctly with the new EXISTS query
• Performance improvements are maintained over time
• Edge cases (empty search, no results, large datasets)

🏁 Recommendation

✅ APPROVE - This is a well-executed performance optimization that improves security and maintains functionality. The changes are focused, follow existing patterns, and address a clear performance bottleneck.

Suggestions for follow-up:

• Monitor query performance in production to validate the improvements
• Consider adding integration tests for the events endpoint
• Document the performance optimization reasoning for future developers

[claude]

Code Review Feedback

Thank you for this performance optimization PR! I've reviewed the changes and here's my feedback:

✅ Positive Changes

Performance Improvements:

• Excellent: Added ClickHouse index on event_props.event_id with proper minmax type and granularity
• Smart query optimization: Removed unnecessary LEFT JOIN and DISTINCT operations
• Parameterized queries: Properly using aliasIds Array prevents SQL injection and improves query planning
• Simplified count query: Changed from count(DISTINCT events.id) to count(*) which is more efficient

Code Quality:

• Good: Consistent alias usage (e for events table)
• Clean: Removed redundant string concatenation for alias IDs
• Proper: Following ClickHouse migration naming convention (007AddEventPropsEventIdIndex.ts)

🔍 Areas for Consideration

Migration:

• The migration looks correct but consider testing it on a large dataset first
• The IF NOT EXISTS clause is good practice for preventing duplicate index creation

Query Logic:

• The search functionality now uses a subquery in the WHERE clause, which should perform better with the new index
• Consider if we need any additional indexes on prop_value for the ILIKE search if this becomes a performance bottleneck

🧪 Test Coverage

The existing tests in tests/services/player/events.test.ts should cover the functional aspects of these changes:

• ✅ Basic event fetching
• ✅ Search functionality
• ✅ Pagination
• ✅ Access control

Recommendation: Consider adding a performance test or benchmark to validate the improvements, especially for large datasets.

🔒 Security

• ✅ SQL Injection: Properly parameterized queries prevent injection attacks
• ✅ Access Control: No changes to existing permission structure

📝 Minor Suggestions

1. Consider adding a comment in the migration explaining why this index improves performance
2. The query structure is much cleaner - good work!

🎯 Overall Assessment

This is a well-executed performance optimization that addresses a clear bottleneck. The changes are focused, safe, and should provide meaningful performance improvements for player event fetching operations.

Recommendation: ✅ Approve - Ready to merge after any final testing on staging environment.

Great work on identifying and fixing this performance issue! 🚀

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.23%. Comparing base (2cb62c6) to head (df414ed).
⚠️ Report is 6 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/services/player.service.ts	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #604      +/-   ##
===========================================
- Coverage    99.24%   99.23%   -0.01%     
===========================================
  Files          208      208              
  Lines        14527    14524       -3     
  Branches      1299     1299              
===========================================
- Hits         14417    14413       -4     
- Misses         107      108       +1     
  Partials         3        3              

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

Code Review Feedback

Great work on optimizing the player events fetching! This PR demonstrates solid performance improvements through better query structure and ClickHouse indexing. Here's my detailed review:

✅ Strengths

Performance Improvements

• Excellent optimization: Replacing JOIN with a subquery and IN clause is a smart ClickHouse optimization
• Window function usage: Using count(*) OVER() to get total count in a single query eliminates the need for a separate count query
• Index addition: Adding event_id_idx on event_props.event_id will significantly improve search performance when filtering by prop values

Code Quality

• Clean query structure: The CTE (WITH filtered_events) improves readability
• Proper parameterization: Using parameterized queries prevents SQL injection
• Type safety: Maintained TypeScript types with ClickHouseEvent & { total_count: string }

⚠️ Areas for Improvement

Potential Bug - Line 349

const count = results.length > 0 ? Number(results[0].total_count) : 0

Issue: This logic assumes all rows have the same total_count, but if pagination returns fewer results than expected, total_count might not reflect the true total.
Recommendation: Consider handling edge cases where results is empty but total count should still be returned.

Migration Naming - Line 1

The migration file 007AddEventPropsEventIdIndex.ts follows the pattern but could be more descriptive.
Suggestion: Consider 007AddEventPropsEventIdMinMaxIndex.ts to specify the index type.

Query Performance Consideration

The subquery pattern is good, but consider if PREWHERE could be used instead of WHERE for the alias filtering in ClickHouse for even better performance:

PREWHERE e.player_alias_id IN ({aliasIds:Array(UInt32)})

🔒 Security Assessment

• ✅ Parameterized queries: Proper use of query parameters prevents SQL injection
• ✅ Input validation: Search parameters are properly escaped with ILIKE pattern matching
• ✅ Access control: Maintains existing permission checks via @HasPermission

🧪 Test Coverage

Good news: Existing tests in tests/services/player/events.test.ts should continue to pass and validate the functionality. The tests cover:

• Basic event fetching
• Search functionality
• Pagination
• Permission checks

Recommendation: Consider adding a performance test to validate the optimization impact.

📊 Performance Impact

The changes should provide:

1. Faster queries: Subquery + IN is more efficient than JOIN for this use case in ClickHouse
2. Reduced query count: Single query instead of two (data + count)
3. Better search performance: New index on event_props.event_id

🎯 Minor Suggestions

1. Consider adding a comment explaining why the subquery pattern was chosen over JOIN
2. The migration could include error handling for index creation failures

Overall Assessment

This is a well-executed performance optimization that maintains code quality and doesn't break existing functionality. The approach is sound for ClickHouse optimization patterns.

Recommendation: ✅ Approve with minor considerations