Baseline Comparison Study — Code Review

Summary

The A/B experiment framework is implemented across 6 files with clean separation of concerns: types and hashing in experiment/mod.rs, report aggregation in experiment/report.rs, public API surface in db/experiment.rs, chronological profile in ranking/builtins.rs, signal index extensions in entities/user_signal.rs, and integration tests in tests/pg1_baseline.rs.

Files Reviewed

File	Status	Notes
`tidal/src/experiment/mod.rs`	PASS	Types, FNV-1a hash, validation, 16 unit tests
`tidal/src/experiment/report.rs`	PASS	ReportBuilder, read-only aggregation, no allocations in hot path
`tidal/src/db/experiment.rs`	PASS	Thin TidalDb wiring, validates config before use
`tidal/src/ranking/builtins.rs`	PASS	chronological() profile, Sort::New, no boosts
`tidal/src/entities/user_signal.rs`	PASS	user_signal_count + user_activity_split added
`tidal/tests/pg1_baseline.rs`	PASS	7 integration tests covering all QA scenarios
`tidal/src/lib.rs`	PASS	Re-exports all public experiment types
`tidal/src/db/mod.rs`	PASS	mod experiment declared

Correctness

FNV-1a hash determinism: Uses fixed offset basis and prime constants. Combines experiment_id bytes followed by user_id little-endian bytes. No platform-dependent behavior. Verified by assign_group_is_deterministic test (1000 iterations).
Balanced split: 10,000 modulo buckets with configurable threshold. Unit tests verify 50/50 and 10/90 splits are within statistical tolerance (+-2.5%).
Division by zero: GroupMetrics handles zero views gracefully (returns 0.0 CTR, not NaN). relative_lift handles zero control explicitly (returns 0.0 when both zero, INFINITY when only control zero).
Config validation: Rejects out-of-range fractions, empty signal lists. Boundary values (0.0, 1.0) accepted. Validation called in all three TidalDb methods before any work.
Report aggregation: Signal type IDs resolved once via resolve_signal_type().ok(), not per-user. Missing signal types (unregistered) are silently skipped, yielding zero counts.
Return rate: Uses user_activity_split to check if user has both historical and recent activity within the return window. Users with no history are excluded from the denominator.

Design Quality

Read-only: The experiment module never writes to storage. All metrics derived from existing signal data.
No new fields on TidalDb: No additional struct fields needed. Uses existing ledger and user_signal_index.
Pure functions where possible: assign_group and fnv1a_hash are pure (no &self, no I/O).
Minimal API surface: Three methods on TidalDb (experiment_group, experiment_profile, experiment_report). Clean re-exports.
Separation of concerns: Types/hashing separate from aggregation separate from TidalDb wiring.

Potential Improvements (Non-blocking)

O(n*m) scan in user_signal_count: Iterates all entries in UserSignalIndex to find matches for a given user. For large deployments, a secondary index keyed by user_id could improve performance. Acceptable for the PG1 pilot scope (hundreds of users).
Return rate approximation: user_activity_split uses Window::OneHour for "recent" detection. This is a heuristic -- users who returned 2 hours ago but not in the last hour would be missed. The design doc acknowledges this trade-off.

Verdict

APPROVED. Clean implementation with thorough test coverage. No correctness issues. The experiment framework is read-only, adds no overhead to the hot path, and integrates cleanly with existing signal infrastructure. Non-blocking improvements noted for future scaling.

3.7 KiB Raw Blame History