allow bounds of {eq: key} #159

ianmacartney · 2025-10-30T02:14:01Z

Currently if your aggregate key is not an array, prefix doesn't work (gives an opaque type error).
Instead, allow doing eq since this is likely what you want.

And don't allow "prefix" when the key is not an array.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Summary by CodeRabbit

New Features
- Added equality bounds for precise query matching on scalar and array keys.
- Introduced prefix-based bounds specifically for array-keyed queries, enabling flexible prefix matching.
Tests
- Comprehensive test coverage added for equality bounds, prefix bounds, batch operations, namespace isolation, and edge-case scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

pkg-pr-new · 2025-10-30T02:15:07Z

Open in StackBlitz

npm i https://pkg.pr.new/get-convex/aggregate/@convex-dev/aggregate@159

commit: 7be2c22

Nicolapps

Thank you! Can you please add a unit test for this?

ianmacartney · 2025-12-09T17:58:06Z

@coderabbitai can you write a few targeted tests here?

coderabbitai · 2025-12-09T17:59:40Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the list of files changed in this PR
gh pr view 159 --json files --jq '.files[].path'

Length of output: 43

🏁 Script executed:

#!/bin/bash
# Get the diff for this PR to understand the changes
gh pr diff 159

Length of output: 1336

🏁 Script executed:

#!/bin/bash
# Find existing test files to understand the test structure
fd -e test.ts -e spec.ts

Length of output: 138

🏁 Script executed:

#!/bin/bash
# Look for test files related to aggregates
fd -e ts . | rg -i test | rg -i aggregate

Length of output: 32

🏁 Script executed:

#!/bin/bash
# Look at the client test file to understand test structure
cat src/client/index.test.ts

Length of output: 28067

🏁 Script executed:

#!/bin/bash
# Look at the positions.ts file to understand the full context
cat src/client/positions.ts

Length of output: 3872

🏁 Script executed:

#!/bin/bash
# Check if there are any existing tests for boundsToPositions
rg -n "boundsToPositions" src/client/

Length of output: 795

@ianmacartney, I'll write targeted tests for the new {eq: key} bounds functionality. Based on the PR changes, I'll test:

{eq: value} with non-array keys (the main new feature)
{eq: [array]} with array keys
Verification that existing prefix functionality still works

Here are the tests to add to src/client/index.test.ts:

describe("Bounds with eq", () => {
  let t: ConvexTest;
  let aggregate: ReturnType<typeof createAggregates>["aggregate"];

  beforeEach(() => {
    t = setupTest();
    ({ aggregate } = createAggregates());
  });

  test("should count items with eq bound on non-array key", async () => {
    await t.run(async (ctx) => {
      const docs = [
        { name: "item1", value: 10 },
        { name: "item2", value: 20 },
        { name: "item3", value: 20 },
        { name: "item4", value: 30 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregate.insert(ctx, insertedDoc!);
      }
    });

    // Test eq bound - should only count items with exact key value
    const countEq20 = await t.run(async (ctx) => {
      return await aggregate.count(ctx, {
        bounds: { eq: 20 },
      });
    });
    expect(countEq20).toBe(2); // Two items with value 20

    const countEq10 = await t.run(async (ctx) => {
      return await aggregate.count(ctx, {
        bounds: { eq: 10 },
      });
    });
    expect(countEq10).toBe(1); // One item with value 10

    const countEq30 = await t.run(async (ctx) => {
      return await aggregate.count(ctx, {
        bounds: { eq: 30 },
      });
    });
    expect(countEq30).toBe(1); // One item with value 30

    const countEq40 = await t.run(async (ctx) => {
      return await aggregate.count(ctx, {
        bounds: { eq: 40 },
      });
    });
    expect(countEq40).toBe(0); // No items with value 40
  });

  test("should sum items with eq bound on non-array key", async () => {
    await t.run(async (ctx) => {
      const docs = [
        { name: "item1", value: 15 },
        { name: "item2", value: 15 },
        { name: "item3", value: 25 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregate.insert(ctx, insertedDoc!);
      }
    });

    const sumEq15 = await t.run(async (ctx) => {
      return await aggregate.sum(ctx, {
        bounds: { eq: 15 },
      });
    });
    expect(sumEq15).toBe(30); // 15 + 15 = 30

    const sumEq25 = await t.run(async (ctx) => {
      return await aggregate.sum(ctx, {
        bounds: { eq: 25 },
      });
    });
    expect(sumEq25).toBe(25);

    const sumEq100 = await t.run(async (ctx) => {
      return await aggregate.sum(ctx, {
        bounds: { eq: 100 },
      });
    });
    expect(sumEq100).toBe(0); // No items with value 100
  });

  test("should work with eq bound on array keys", async () => {
    const aggregateWithArrayKeys = new TableAggregate(components.aggregate, {
      sortKey: (doc) => [doc.value, doc.name],
      sumValue: (doc) => doc.value,
    });

    await t.run(async (ctx) => {
      const docs = [
        { name: "a", value: 10 },
        { name: "b", value: 20 },
        { name: "c", value: 20 },
        { name: "d", value: 30 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregateWithArrayKeys.insert(ctx, insertedDoc!);
      }
    });

    // Test eq bound with exact array match
    const countEqArray = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { eq: [20, "b"] },
      });
    });
    expect(countEqArray).toBe(1); // Only one item with exact key [20, "b"]

    const countEqArray2 = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { eq: [20, "c"] },
      });
    });
    expect(countEqArray2).toBe(1); // Only one item with exact key [20, "c"]

    const countEqArrayNonExistent = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { eq: [20, "z"] },
      });
    });
    expect(countEqArrayNonExistent).toBe(0); // No items with key [20, "z"]
  });

  test("should work with eq bound and namespace", async () => {
    const { aggregateWithNamespace } = createAggregates();

    await t.run(async (ctx) => {
      const docs = [
        { album: "vacation", url: "photo1.jpg", score: 10 },
        { album: "vacation", url: "photo2.jpg", score: 20 },
        { album: "vacation", url: "photo3.jpg", score: 20 },
        { album: "family", url: "photo4.jpg", score: 20 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("photos", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregateWithNamespace.insert(ctx, insertedDoc!);
      }
    });

    // Test eq bound within vacation namespace
    const vacationCountEq20 = await t.run(async (ctx) => {
      return await aggregateWithNamespace.count(ctx, {
        namespace: "vacation",
        bounds: { eq: 20 },
      });
    });
    expect(vacationCountEq20).toBe(2); // Two vacation photos with score 20

    // Test eq bound within family namespace
    const familyCountEq20 = await t.run(async (ctx) => {
      return await aggregateWithNamespace.count(ctx, {
        namespace: "family",
        bounds: { eq: 20 },
      });
    });
    expect(familyCountEq20).toBe(1); // One family photo with score 20

    // Test eq bound with non-existent value in namespace
    const vacationCountEq100 = await t.run(async (ctx) => {
      return await aggregateWithNamespace.count(ctx, {
        namespace: "vacation",
        bounds: { eq: 100 },
      });
    });
    expect(vacationCountEq100).toBe(0);
  });

  test("should use eq bounds in batch operations", async () => {
    await t.run(async (ctx) => {
      const docs = [
        { name: "item1", value: 10 },
        { name: "item2", value: 20 },
        { name: "item3", value: 20 },
        { name: "item4", value: 30 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregate.insert(ctx, insertedDoc!);
      }
    });

    // Test countBatch with multiple eq bounds
    const counts = await t.run(async (ctx) => {
      return await aggregate.countBatch(ctx, [
        { bounds: { eq: 10 } },
        { bounds: { eq: 20 } },
        { bounds: { eq: 30 } },
        { bounds: { eq: 40 } },
      ]);
    });
    expect(counts).toEqual([1, 2, 1, 0]);

    // Test sumBatch with eq bounds
    const sums = await t.run(async (ctx) => {
      return await aggregate.sumBatch(ctx, [
        { bounds: { eq: 10 } },
        { bounds: { eq: 20 } },
        { bounds: { eq: 30 } },
      ]);
    });
    expect(sums).toEqual([10, 40, 30]);
  });
});

describe("Bounds with prefix on array keys", () => {
  let t: ConvexTest;

  beforeEach(() => {
    t = setupTest();
  });

  test("should still work with prefix bounds on array keys", async () => {
    const aggregateWithArrayKeys = new TableAggregate(components.aggregate, {
      sortKey: (doc) => [doc.value, doc.name],
      sumValue: (doc) => doc.value,
    });

    await t.run(async (ctx) => {
      const docs = [
        { name: "a", value: 10 },
        { name: "b", value: 10 },
        { name: "c", value: 20 },
        { name: "d", value: 20 },
        { name: "e", value: 30 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregateWithArrayKeys.insert(ctx, insertedDoc!);
      }
    });

    // Test prefix bound - should match all items with value 10
    const countPrefix10 = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { prefix: [10] },
      });
    });
    expect(countPrefix10).toBe(2); // Two items with first element 10

    const countPrefix20 = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { prefix: [20] },
      });
    });
    expect(countPrefix20).toBe(2); // Two items with first element 20

    // Test empty prefix - should match all items
    const countPrefixEmpty = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { prefix: [] },
      });
    });
    expect(countPrefixEmpty).toBe(5); // All items

    // Test full prefix - should match exact item
    const countPrefixFull = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.count(ctx, {
        bounds: { prefix: [10, "a"] },
      });
    });
    expect(countPrefixFull).toBe(1); // Only [10, "a"]
  });

  test("should sum with prefix bounds on array keys", async () => {
    const aggregateWithArrayKeys = new TableAggregate(components.aggregate, {
      sortKey: (doc) => [doc.value, doc.name],
      sumValue: (doc) => doc.value,
    });

    await t.run(async (ctx) => {
      const docs = [
        { name: "a", value: 10 },
        { name: "b", value: 10 },
        { name: "c", value: 20 },
      ];

      for (const doc of docs) {
        const id = await ctx.db.insert("testItems", doc);
        const insertedDoc = await ctx.db.get(id);
        await aggregateWithArrayKeys.insert(ctx, insertedDoc!);
      }
    });

    const sumPrefix10 = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.sum(ctx, {
        bounds: { prefix: [10] },
      });
    });
    expect(sumPrefix10).toBe(20); // 10 + 10 = 20

    const sumPrefix20 = await t.run(async (ctx) => {
      return await aggregateWithArrayKeys.sum(ctx, {
        bounds: { prefix: [20] },
      });
    });
    expect(sumPrefix20).toBe(20);
  });
});

These tests cover:

eq with non-array keys (the main new feature from the PR) - testing count, sum, and with namespaces
eq with array keys - ensuring it works for exact array matches
eq in batch operations - countBatch and sumBatch
prefix still works with array keys - verifying the existing functionality isn't broken

The tests follow the existing patterns in the test file and should provide good coverage of the new functionality.

coderabbitai · 2026-01-09T02:20:00Z

📝 Walkthrough

Walkthrough

The changes extend type exports and add comprehensive test coverage for bounds functionality, including equality-based and prefix-based bound operations on both scalar and array keys. A conditional type refinement in the Bounds definition now distinguishes between array-key and non-array-key variants, with corresponding runtime logic updates in boundsToPositions.

Changes

Cohort / File(s)	Summary
Type Exports `src/client/index.ts`	Exported type signature updated to include `Bounds` alongside existing `Key` and `Bound` exports.
Bounds Type & Logic `src/client/positions.ts`	Updated `Bounds` type with conditional branch for array-key prefix support (only when K extends `unknown[]`). Enhanced `boundsToPositions` function to handle `eq` property on bounds, generating symmetric lower/upper positions for equality constraints.
Test Coverage `src/client/index.test.ts`	Extensive test suite added covering: eq-bound scenarios on non-array and array-key tables with count/sum validations; namespace isolation with vacation vs. family data; batch operations (countBatch/sumBatch); prefix-based bounds on array-key tables with various prefix lengths and empty/full-prefix edge cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Bounds now leap with prefix and eq,
Array keys aligned in perfect set,
Tests hop through namespaces bright,
Equality constraints held tight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'allow bounds of {eq: key}' clearly and accurately describes the main feature added: support for equality bounds with {eq: key} syntax, which aligns with the core objective of the PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/client/positions.ts (2)

83-110: eq → inclusive lower+upper positions looks correct; consider making Bounds variants mutually exclusive.

The eq mapping to [key, null..[]] (via boundToPosition) should include all IDs for that key, which matches expected “exact key match” semantics. One small footgun: because TS allows extra props, { eq, lower, upper } can still typecheck (assignable to {eq: K}), and runtime will silently prioritize eq. If you want to harden this, you can add never fields to enforce exclusivity.

21-37: Prefix bounds support is working correctly for fixed-length tuple keys; degeneration risk for variable-length arrays is theoretical.

The TuplePrefix type functions as designed for fixed-length tuples (e.g., [number, string]). All test cases and examples use fixed-length tuples, and prefix bounds work reliably in practice. While theoretically a variable-length array key would cause TuplePrefix to degenerate to [] only (since number extends number never terminates), this scenario does not occur in the codebase. If runtime validation or documentation explicitly constrains keys to tuples, no change is needed; if variable-length arrays should remain supported, consider the proposed conditional type to handle both cases.

src/client/index.test.ts (1)

886-1110: Good functional coverage for { bounds: { eq } } (scalar, array, namespace, batch).

One gap: the PR objective also says “prevent prefix when key is not an array” — that’s primarily a type-level guarantee. Consider adding a compile-time assertion in this test file (or a tsd-style test) using // @ts-expect-error to lock in that { prefix: ... } is rejected for Key: number.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37cbcf9 and 7be2c22.

📒 Files selected for processing (3)

src/client/index.test.ts
src/client/index.ts
src/client/positions.ts

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: get-convex/aggregate PR: 0
File: example/.cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T18:51:23.434Z
Learning: Applies to example/convex/**/*.{ts,tsx} : Use array validators with `v.array()` to specify element types in Convex function arguments and schemas

Learnt from: CR
Repo: get-convex/aggregate PR: 0
File: example/.cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T18:51:23.434Z
Learning: Applies to example/convex/**/*.{ts,tsx} : Always define arrays as `const array: Array<T> = [...]` with explicit type annotation

📚 Learning: 2025-12-03T18:51:23.434Z

Learnt from: CR
Repo: get-convex/aggregate PR: 0
File: example/.cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T18:51:23.434Z
Learning: Applies to example/convex/**/*.{ts,tsx} : Index fields must be queried in the same order they are defined; create separate indexes if you need different query orders

Applied to files:

src/client/index.test.ts

📚 Learning: 2025-12-03T18:51:23.434Z

Learnt from: CR
Repo: get-convex/aggregate PR: 0
File: example/.cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T18:51:23.434Z
Learning: Applies to example/convex/**/*.{ts,tsx} : Always define arrays as `const array: Array<T> = [...]` with explicit type annotation

Applied to files:

src/client/positions.ts

📚 Learning: 2025-12-03T18:51:23.434Z

Learnt from: CR
Repo: get-convex/aggregate PR: 0
File: example/.cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T18:51:23.434Z
Learning: Applies to example/convex/**/*.{ts,tsx} : Always define records as `const record: Record<KeyType, ValueType> = {...}` with explicit type annotation

Applied to files:

src/client/positions.ts

🧬 Code graph analysis (1)

src/client/index.test.ts (3)

src/component/schema.ts (1)

aggregate (25-28)

src/client/index.ts (1)

TableAggregate (737-888)

src/client/setup.test.ts (1)

components (13-15)

🔇 Additional comments (2)

src/client/index.ts (1)

37-37: Exporting Bounds is the right public-API follow-through.

Keeps the new bounds shapes consumable by downstream code without importing from internal modules.

src/client/index.test.ts (1)

1111-1214: Prefix regression tests on array keys look solid (empty, partial, full prefix; count+sum).

This should catch accidental behavior changes in boundsToPositions for the prefix path while the new eq path is added.

ianmacartney mentioned this pull request Oct 30, 2025

Aggregates definitions and their respective queries must be in the same file #25

Open

ianmacartney requested a review from Nicolapps December 5, 2025 03:52

Nicolapps approved these changes Dec 9, 2025

View reviewed changes

ianmacartney added 2 commits January 8, 2026 18:19

allow bounds of {eq: key}

0108711

add tests

7be2c22

ianmacartney force-pushed the ian/eq-bounds branch from 69691cd to 7be2c22 Compare January 9, 2026 02:19

coderabbitai bot reviewed Jan 9, 2026

View reviewed changes

ianmacartney merged commit 7be2c22 into main Jan 9, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

allow bounds of {eq: key} #159

allow bounds of {eq: key} #159

Uh oh!

ianmacartney commented Oct 30, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

pkg-pr-new bot commented Oct 30, 2025 •

edited

Loading

Uh oh!

Nicolapps left a comment

Uh oh!

ianmacartney commented Dec 9, 2025

Uh oh!

coderabbitai bot commented Dec 9, 2025

Uh oh!

coderabbitai bot commented Jan 9, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

allow bounds of {eq: key} #159

allow bounds of {eq: key} #159

Uh oh!

Conversation

ianmacartney commented Oct 30, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

pkg-pr-new bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nicolapps left a comment

Choose a reason for hiding this comment

Uh oh!

ianmacartney commented Dec 9, 2025

Uh oh!

coderabbitai bot commented Dec 9, 2025

Uh oh!

coderabbitai bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ianmacartney commented Oct 30, 2025 •

edited by coderabbitai bot

Loading

pkg-pr-new bot commented Oct 30, 2025 •

edited

Loading

coderabbitai bot commented Jan 9, 2026 •

edited

Loading