Namespace aggregates #20

ldanilek · 2024-11-04T18:03:21Z

Add namespacing to aggregates.

All of the data and even the APIs should be backwards-compatible.

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

ianmacartney

Very cool. Since namespace is being baked into the base Aggregate classes, I'd rather not have separate classes, and just have a required argument to the base function if namespace's type is not undefined.
And then we can (optionally) have a .for helper if you want to specify the namespace once and have an api that injects it, along with some fancy types so the argument is (1) required if you specified a namespace document fn but didn't pass one to the constructure, (2) not allowed if you didn't specify a namespace fn, or already specified the namespace via for.

Are you game for that?

README.md

ianmacartney · 2024-11-05T01:54:18Z

README.md

+  [string, number],
+  DataModel,
+  "leaderboard",
+  Id<"games">


I think we should use an object type param here if it can also work. Knowing what each parameter means is hard. What do you think of something like:

NamespacedTableAggregate<{ sortKey: [string, number], DataModel: DataModel, // maybe just `DataModel,` works? tableName: "leaderboard", namespace: Id<"games"> }>

I also tried for a bit but couldn't get working: having this type inferred from the parameters somehow. e.g. the namespace and sortKey types should be inferable somehow, since their return values below dictate the types. But I couldn't get it working when I tried

ianmacartney · 2024-11-05T01:57:29Z

README.md

+Now when you need to aggregate within a game, you call `.get` to narrow down the
+computation to a single game.
+
+```ts
+const countTimesGamePlayed = await aggregateByGame.get(gameId).count();


Suggested change

Now when you need to aggregate within a game, you call `.get` to narrow down the

computation to a single game.

```ts

const countTimesGamePlayed = await aggregateByGame.get(gameId).count();

Now when you need to aggregate within a game, you call `.for` to narrow down the

computation to a single game.

```ts

const countTimesGamePlayed = await aggregateByGame.for(gameId).count();

ianmacartney · 2024-11-05T01:57:59Z

example/convex/photos.ts

-  handler: async (ctx, { offset, numItems }) => {
-    const { key: firstPhotoCreationTime } = await photos.at(ctx, offset);
+  handler: async (ctx, { offset, numItems, album }) => {
+    const { key: firstPhotoCreationTime } = await photos.get(album).at(ctx, offset);


Suggested change

const { key: firstPhotoCreationTime } = await photos.get(album).at(ctx, offset);

const { key: firstPhotoCreationTime } = await photos.for(album).at(ctx, offset);

ianmacartney · 2024-11-05T02:00:56Z

src/client/index.ts

+    protected component: UsedAPI,
+  ) {}
+
+  get(namespace: Namespace): Aggregate<K, ID, Namespace> {


Suggested change

get(namespace: Namespace): Aggregate<K, ID, Namespace> {

for(namespace: Namespace): Aggregate<K, ID, Namespace> {

src/component/public.ts

ianmacartney · 2024-11-05T02:11:59Z

src/component/inspect.ts

@@ -1,12 +1,12 @@
 import { v } from "convex/values";
 import { Id } from "./_generated/dataModel.js";
 import { DatabaseReader, query } from "./_generated/server.js";
-import { getTree, p } from "./btree.js";
+import { getTree, Namespace, p } from "./btree.js";

 export const display = query({


wow I've missed the existence of this - would be nice to document how to view / debug your state in the readme. One common thing for me is trying to figure out why some documents ended up busted - and finding myself crawling through the dashboard data to try to look for the data ordering. another utility I've wanted is for table aggregate: "refresh this document with the current doc value / sort key" - and a backfill utility to just crawl a table and re-calculate every doc would be handy in dev. maybe just copying from your example is enough there though

src/component/btree.ts

Co-authored-by: Ian Macartney <[email protected]>

ianmacartney

looks good. The type names should be TitleCase even as object keys (I believe). I started adding inline suggestions, but doing a find & replace in an editor will likely be faster

ianmacartney · 2024-11-06T07:25:40Z

README.md

+  namespace: Id<"games">,
+  key: number,
+  dataModel: DataModel,
+  tableName: "scores",


Suggested change

namespace: Id<"games">,

key: number,

dataModel: DataModel,

tableName: "scores",

Namespace: Id<"games">,

Key: number,

DataModel: DataModel,

TableName: "scores",

and I'm not sure if that allows you to do just DataModel,

It does not allow that. While i'm find-replacing, i was wondering if Key should be called SortKey

ianmacartney · 2024-11-06T07:26:28Z

README.md

+  namespace: undefined,
+  key: number,
+  dataModel: DataModel,
+  tableName: "mytable",


Suggested change

namespace: undefined,

key: number,

dataModel: DataModel,

tableName: "mytable",

Namespace: undefined,

Key: number,

DataModel: DataModel,

TableName: "mytable",

ianmacartney · 2024-11-06T07:27:04Z

README.md

+  namespace: Id<"games">,
+  key: [string, number],
+  dataModel: DataModel,
+  tableName: "leaderboard"


Suggested change

namespace: Id<"games">,

key: [string, number],

dataModel: DataModel,

tableName: "leaderboard"

Namespace: Id<"games">,

Key: [string, number],

DataModel: DataModel,

TableName: "leaderboard"

ianmacartney · 2024-11-06T07:27:21Z

README.md

+  namespace: undefined,
+  key: null,
+  dataModel: DataModel,
+  tableName: "mytable",


Suggested change

namespace: undefined,

key: null,

dataModel: DataModel,

tableName: "mytable",

Namespace: undefined,

Key: null,

DataModel: DataModel,

TableName: "mytable",

ianmacartney · 2024-11-06T07:28:01Z

README.md

+  namespace: string,  // album name
+  key: number,  // creation time
+  dataModel: DataModel,
+  tableName: "photos",


Suggested change

namespace: string, // album name

key: number, // creation time

dataModel: DataModel,

tableName: "photos",

Namespace: string, // album name

Key: number, // creation time

DataModel: DataModel,

TableName: "photos",

ianmacartney · 2024-11-06T07:30:46Z

src/client/index.ts


  /// Aggregate queries.

  /**
   * Counts items between the given bounds.
   */
-  async count(ctx: RunQueryCtx, bounds?: Bounds<K, ID>): Promise<number> {
+  async count(ctx: RunQueryCtx, ...opts: NamespacedOpts<{ bounds: Bounds<K, ID> }, Namespace>): Promise<number> {


Suggested change

async count(ctx: RunQueryCtx, ...opts: NamespacedOpts<{ bounds: Bounds<K, ID> }, Namespace>): Promise<number> {

async count(ctx: RunQueryCtx, ...opts: NamespacedOpts<{ Bounds: Bounds<K, ID> }, Namespace>): Promise<number> {

I don't think this one should be TitleCase. That would mean you call it like aggregate.count(ctx, { Bounds: { prefix: [username] } })

ianmacartney · 2024-11-06T07:36:06Z

src/client/index.ts

+  key: K,
+  dataModel: DataModel,
+  tableName: TableName,
+  namespace: Namespace,


Suggested change

key: K,

dataModel: DataModel,

tableName: TableName,

namespace: Namespace,

Key: K,

DataModel: DataModel,

TableName: TableName,

Namespace: Namespace,

ianmacartney · 2024-11-06T07:40:46Z

src/client/index.ts

@@ -657,3 +745,31 @@ export function btreeItemToAggregateItem<K extends Key, ID extends string>({
    sumValue: s,
  };
 }
+
+export type NamespacedArgs<Args, Namespace> =
+  Args & { namespace: Namespace } | (Namespace extends undefined ? Args : never);


so you can provide namespace: undefined if you didn't specify one? I wonder if this works if that's not intended:

Args & (Namespace extends undefined ? object : { namespace: Namespace });

That is intended. It's useful for methods like min that call other methods and pass in the namespace.

ianmacartney · 2024-11-06T07:43:05Z

src/client/index.ts

+export type NamespacedOpts<Opts, Namespace> =
+  [{ namespace: Namespace } & Opts] | (
+    Namespace extends undefined
+      ? [] | [Opts]


This might be possible to do with [Opts?]

TIL that syntax

README.md

Co-authored-by: Ian Macartney <[email protected]>

into lee/namespaces

ianmacartney · 2024-11-06T17:40:22Z

Sounds good. And SortKey does sound better

…

On Wed, Nov 6, 2024 at 9:33 AM Lee Danilek ***@***.***> wrote: Merged #20 <#20> into main. — Reply to this email directly, view it on GitHub <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZQW3VMX5YJW3D5EMC7STZ7JHINAVCNFSM6AAAAABREZ7VZ2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJVGE2TKMRXHE4TONY> . You are receiving this because your review was requested.Message ID: ***@***.***>

.

0ed7ff5

ldanilek requested a review from ianmacartney November 4, 2024 18:03

.

a81fcb6

ianmacartney reviewed Nov 5, 2024

View reviewed changes

ldanilek and others added 7 commits November 5, 2024 11:19

Update README.md

1720eb5

Co-authored-by: Ian Macartney <[email protected]>

Update README.md

bf1d1e5

Co-authored-by: Ian Macartney <[email protected]>

Update README.md

1c37f29

Co-authored-by: Ian Macartney <[email protected]>

remove Randomize and use object as type param

44fcbb7

change arguments to be objects

54ffc84

.

3fbe616

upgrade convex-test and fix test

f32eec8

ldanilek requested a review from ianmacartney November 6, 2024 02:32

.

473e136

ianmacartney approved these changes Nov 6, 2024

View reviewed changes

ldanilek and others added 4 commits November 6, 2024 08:27

Update README.md

0b7226f

Co-authored-by: Ian Macartney <[email protected]>

prettier and TitleCase

49eeac2

Merge branch 'lee/namespaces' of https://github.com/get-convex/aggregate

3ecea7e

into lee/namespaces

.

67cbe91

ldanilek merged commit d4f882c into main Nov 6, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespace aggregates #20

Namespace aggregates #20

ldanilek commented Nov 4, 2024

ianmacartney left a comment

ianmacartney Nov 5, 2024

ianmacartney Nov 5, 2024

ianmacartney Nov 5, 2024

ianmacartney Nov 5, 2024

ianmacartney Nov 5, 2024

ianmacartney Nov 5, 2024

ianmacartney left a comment

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ldanilek Nov 6, 2024

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ldanilek Nov 6, 2024

ianmacartney Nov 6, 2024

ianmacartney Nov 6, 2024

ldanilek Nov 6, 2024

ianmacartney Nov 6, 2024

ldanilek Nov 6, 2024

ianmacartney commented Nov 6, 2024 via email

	const { key: firstPhotoCreationTime } = await photos.get(album).at(ctx, offset);
	const { key: firstPhotoCreationTime } = await photos.for(album).at(ctx, offset);

	get(namespace: Namespace): Aggregate<K, ID, Namespace> {
	for(namespace: Namespace): Aggregate<K, ID, Namespace> {

	async count(ctx: RunQueryCtx, ...opts: NamespacedOpts<{ bounds: Bounds<K, ID> }, Namespace>): Promise<number> {
	async count(ctx: RunQueryCtx, ...opts: NamespacedOpts<{ Bounds: Bounds<K, ID> }, Namespace>): Promise<number> {

Namespace aggregates #20

Namespace aggregates #20

Conversation

ldanilek commented Nov 4, 2024

ianmacartney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmacartney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmacartney commented Nov 6, 2024 via email