From ecac94b4e4da157a8e2ecd09c06580d86eb8c27a Mon Sep 17 00:00:00 2001 From: devgbmuyiwa <142212982+devgbmuyiwa@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:47:03 +0100 Subject: [PATCH] feat(ab-testing): add indexes to experiment-variant entity (#1223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `experiment-variant.entity.ts` declared no `@Index`, so every lookup on `experiment_variants` forced a sequential scan regardless of table size. Query-path review (ab-testing.service.ts, experiments/experiment.service.ts, analysis/statistical-analysis.service.ts, automation/automated-decision.service.ts, reporting/ab-testing-reports.service.ts) shows every real access to variant data goes through the `experiment` relation (`experimentRepository.findOne({ relations: [...] })`), then filters the loaded array in-memory for the control variant (`isControl`) or the winning variant (`isWinner`). There is no direct `variantRepository.find({ where })` call on any other column. Changes: - src/ab-testing/entities/experiment-variant.entity.ts: add two class-level composite indexes, `@Index(['experiment', 'isControl'])` and `@Index(['experiment', 'isWinner'])`, matching that access shape. A plain `experiment`-only index is intentionally omitted: leftmost-prefix lookup means either composite already serves a bare "all variants for this experiment" query, so a third index would be a redundant duplicate per the issue's acceptance criteria. A standalone index on `isControl` or `isWinner` alone would also be poor: both are low-cardinality booleans with no selectivity without the `experiment` prefix. - src/migrations/1802000000000-add-experiment-variant-indexes.ts: new migration creating the same two indexes via `CREATE INDEX IF NOT EXISTS ... ("experimentId", "isControl"/"isWinner")` (idempotent, matching this repo's established index-migration style, e.g. 1801000000000-add-achievement-indexes.ts), with a `down()` that drops them. Timestamp/class name follow migration-timestamps.spec.ts's rules (unique timestamp, class name suffixed with it). `experimentId` is the column name confirmed by TypeORM's default FK-naming convention, verified against an already-applied migration in this codebase (1750000000000-add-gamification-indexes.ts) since this entity has no custom NamingStrategy or explicit @JoinColumn. - Migrations are auto-discovered via the `src/migrations/[0-9]*.{ts,js}` glob in src/config/datasource.ts, so no manual registration was needed. `synchronize` is `false` there, so the migration — not the `@Index` decorators — is what actually creates the indexes on a real database; the decorators keep the entity's schema intent documented and in sync. Validation: node_modules is not installed in this sandbox (only a single stray entry under node_modules/, `jest`/`tsc` not resolvable), so `npm test`, `npm run typecheck`, and `npm run lint` could not be run here. Both files were reviewed by hand against this repo's real conventions: the migration's timestamp/class-name were checked against migration-timestamps.spec.ts's actual assertions, the FK column name was cross-checked against a real applied migration rather than assumed, and the entity's decorator/import syntax mirrors user-achievement.entity.ts's established `@Index([...])` composite-index pattern in this same codebase. Co-Authored-By: Claude Sonnet 5 --- .../entities/experiment-variant.entity.ts | 32 +++++++++++++++++ ...00000000-add-experiment-variant-indexes.ts | 35 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 src/migrations/1802000000000-add-experiment-variant-indexes.ts diff --git a/src/ab-testing/entities/experiment-variant.entity.ts b/src/ab-testing/entities/experiment-variant.entity.ts index 00c68e36..cf7ab129 100644 --- a/src/ab-testing/entities/experiment-variant.entity.ts +++ b/src/ab-testing/entities/experiment-variant.entity.ts @@ -8,14 +8,46 @@ import { ManyToOne, OneToMany, VersionColumn, + Index, } from 'typeorm'; import { Experiment } from './experiment.entity'; import { VariantMetric } from './variant-metric.entity'; /** * Represents the experiment Variant entity. + * + * ## Indexes (#1223) + * + * Every real query path against this entity loads variants through the + * `experiment` relation (`experimentRepository.findOne({ relations: [...] })` + * across `ab-testing.service.ts`, `experiments/experiment.service.ts`, + * `analysis/statistical-analysis.service.ts`, + * `automation/automated-decision.service.ts`, and + * `reporting/ab-testing-reports.service.ts`), then filters the loaded array + * in-memory for the control variant (`v.isControl`) or the winning variant + * (`v.isWinner`) — every one of those service methods does this. There is + * no direct `variantRepository.find({ where: ... })` call anywhere in this + * codebase filtering on anything else. + * + * The two composite indexes below match that access shape directly — + * `(experiment, isControl)` and `(experiment, isWinner)` — rather than a + * plain single-column index on `experiment` alone: leftmost-prefix lookup + * means either composite already serves a plain "all variants for this + * experiment" query just as well as a dedicated `experiment`-only index + * would, so adding one of those *in addition* would only be a redundant, + * un-selective duplicate (`isControl`/`isWinner` are booleans — indexing + * either alone, without the `experiment` prefix, has essentially no + * selectivity and no query in this codebase would use it that way). + * + * See `src/migrations/1802000000000-add-experiment-variant-indexes.ts` for + * the migration that creates the same two indexes on an existing database + * (`synchronize` is `false` in `src/config/datasource.ts`, so the + * `@Index` decorators below are the schema's documentation of intent, not + * what actually creates the indexes). */ @Entity({ name: 'experiment_variants' }) +@Index(['experiment', 'isControl']) +@Index(['experiment', 'isWinner']) export class IExperimentVariant { @PrimaryGeneratedColumn('uuid') id: string; diff --git a/src/migrations/1802000000000-add-experiment-variant-indexes.ts b/src/migrations/1802000000000-add-experiment-variant-indexes.ts new file mode 100644 index 00000000..cc66c70a --- /dev/null +++ b/src/migrations/1802000000000-add-experiment-variant-indexes.ts @@ -0,0 +1,35 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * #1223 — src/ab-testing/entities/experiment-variant.entity.ts declared no + * indexes. Every real query path against `experiment_variants` loads + * variants through the `experiment` foreign key (via the `experiment` + * relation on `IExperimentVariant`, e.g. + * `experimentRepository.findOne({ relations: [...] })` across the + * ab-testing services), then filters in-memory for the control variant + * (`isControl`) or the winning variant (`isWinner`). These two composite + * indexes match that shape directly. See the `@Index` decorators and their + * accompanying comment on `IExperimentVariant` for the full rationale, + * including why this is two composites rather than three separate indexes + * (a plain `experimentId`-only index would be redundant given either + * composite already serves that lookup via its leftmost column). + */ +export class AddExperimentVariantIndexes1802000000000 implements MigrationInterface { + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + 'CREATE INDEX IF NOT EXISTS "IDX_experiment_variants_experiment_isControl" ON "experiment_variants" ("experimentId", "isControl")', + ); + await queryRunner.query( + 'CREATE INDEX IF NOT EXISTS "IDX_experiment_variants_experiment_isWinner" ON "experiment_variants" ("experimentId", "isWinner")', + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + 'DROP INDEX IF EXISTS "IDX_experiment_variants_experiment_isWinner"', + ); + await queryRunner.query( + 'DROP INDEX IF EXISTS "IDX_experiment_variants_experiment_isControl"', + ); + } +}