feat: tree index api prototype #22491

jenn-le · 2024-09-12T23:22:40Z

Description

TreeIndex is an interface based off of ReadonlyMap that indexes can adhere to
AnchorTreeIndex indexes anchors nodes on given keys that can be customized per schema
createSimpleTreeIndex is a public api that allows users to define the schemas and related keyfinders they want to index on, indexing TreeNodes
createIdentifierIndex creates an index that keys nodes on their identifiers

TODO expand out this description if we want to use this for the API review but maybe it'd be better to just review off the doc?

Reviewer Guidance

The review process is outlined on this wiki page.

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts

taylorsw04 · 2024-09-13T21:38:26Z

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts

+					const nodes = this.nodes.get(key);
+					if (nodes !== undefined) {
+						// if the key already exists in the index, the anchor node is appended to its list of nodes
+						this.nodes.set(key, [...nodes, anchorNode]);


any reason to clone the array rather than push to it?

the array is readonly for api purposes and gets super ugly (not even sure if it's completely feasible) to make a bunch of casts

don't we just need the output type (the thing we hand to getValue) to implement readonly array...which array does? can we not just type our internal type as arry, e.g. private readonly nodes = new Map<TKey, AnchorNode[]>();

we filter the array (meaning allocate a new one) before handing it to getValue anyway, right?

oh yeah, you're right, I was just doing something stupid when I tried it the first time

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts

taylorsw04 · 2024-09-13T22:19:05Z

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts

+		}
+
+		// index any new trees that are created later
+		forest.on("afterRootFieldCreated", (fieldKey) => {


is this event guaranteed to fire for all content created in the tree, including modifications of value fields (e.g. user does foo.x = 5, and the x field is indexed)?

It is not, I created a skipped test and added a todo to use a different event. Will add the proposal for it in the api doc

wait, so are indexes not totally broken until that works? is there another forest event that is fired when that happens that we can subscribe to?

@noencke is claiming that primitives are wrapped in nodes and so even value nodes start out detached and are moved in. this sounds vaguely familiar. have you tested this?

ok, that's what I thought too but then I wrote that skipped test to make sure and it failed

github-actions · 2024-09-16T19:08:56Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  405680 links
    3151 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

taylorsw04 · 2024-09-16T20:57:42Z

packages/dds/tree/api-report/tree.public.api.md

@@ -27,6 +27,9 @@ export interface CommitMetadata {
    readonly kind: CommitKind;
 }

+// @public (undocumented)
+export function createSimpleTreeIndex<TKey extends TreeValue, TValue, TSchema extends TreeNodeSchema>(context: FlexTreeContext, indexer: (schema: TSchema) => KeyFinder<TKey> | undefined, getValue: (nodes: TreeIndexNodes<NodeFromSchema<TSchema>>) => TValue, indexableSchema: readonly TSchema[]): SimpleTreeIndex<TKey, TValue>;


I assume these will go away (public and beta)?

taylorsw04 · 2024-09-16T20:59:26Z

packages/dds/tree/api-report/tree.alpha.api.md

+export function createIdentifierIndex(context: FlexTreeContext): IdentifierIndex;
+
+// @alpha
+export function createSimpleTreeIndex<TKey extends TreeValue, TValue>(context: FlexTreeContext, indexer: (schema: TreeNodeSchema) => KeyFinder<TKey> | undefined, getValue: (nodes: TreeIndexNodes<TreeNode>) => TValue): SimpleTreeIndex<TKey, TValue>;


I don't think we want this in alpha in the current form. Having users provide a KeyFinder (and take the burden of determinism/inval) seems like a bug pit. I feel like a reduced scope form (maybe they provide simple tree schema type + a value field pair and we have a KeyFinder that knows how to handle those) should be exposed first.

We definitely can't expose it until the value field mutation inval issue is solved (if it exists at all)

packages/dds/tree/src/test/simple-tree/identifierIndex.spec.ts

taylorsw04 · 2024-09-16T21:09:32Z

packages/dds/tree/src/test/feature-libraries/indexing/treeIndex.spec.ts

+	return value;
+}
+
+describe.only("tree indexes", () => {


we should have a clear-box test that checks that nodes that are GC'd after being removed are deleted from the index (triggered by "afterDestroy")

taylorsw04 · 2024-09-16T21:25:33Z

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts

+
+					this.anchors.set(anchorNode, anchor);
+					// when the anchor node is destroyed, delete it from the index
+					anchorNode.on("afterDestroy", () => {


having talked to Noah, we think this might be better written as a delta visitor instead of event subscriptions

jenn-le added 29 commits July 30, 2024 00:10

add noah's code

25b0e1f

Merge remote-tracking branch 'upstream/main' into index-api

9a701be

format

7fa4244

start trying some type stuff

f4e3ced

format

8a1224b

some fixes

9587173

some type inference fixes

c305255

add some comments

1d73679

some more comments

bcf4b4f

more comments

28ef175

more comments

ad8c52a

Merge remote-tracking branch 'upstream/main' into index-api

4bea967

more cleanup

eb274ce

Merge remote-tracking branch 'upstream/main' into index-api

8f84ef5

some more comments

849121a

some updates

80e77a2

some type fixes

9f41b27

Merge remote-tracking branch 'upstream/main' into index-api

db696d0

format

95a78ef

comment out stuff

35bbda8

Merge remote-tracking branch 'upstream/main' into index-api

3ea7711

stuff

2ab3e95

stuff

42844b6

Merge remote-tracking branch 'upstream/main' into index-api

ef70f40

stuff

d9eaa93

stuff

6b2075c

Merge remote-tracking branch 'upstream/main' into index-api

4b07bbf

stuff to get tests working

25e29b1

fix deps and get tests passing

a38f598

github-actions bot added the area: dds Issues related to distributed data structures label Sep 12, 2024

github-actions bot added area: dds: tree base: main PRs targeted against main branch labels Sep 12, 2024

jenn-le added 2 commits September 13, 2024 19:15

some changes

459d17c

Merge remote-tracking branch 'upstream/main' into index-api

8dee5df

taylorsw04 reviewed Sep 13, 2024

View reviewed changes

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts Show resolved Hide resolved

taylorsw04 reviewed Sep 13, 2024

View reviewed changes

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts Outdated Show resolved Hide resolved

taylorsw04 reviewed Sep 13, 2024

View reviewed changes

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts Outdated Show resolved Hide resolved

taylorsw04 reviewed Sep 13, 2024

View reviewed changes

packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts Outdated Show resolved Hide resolved

taylorsw04 reviewed Sep 13, 2024

View reviewed changes

jenn-le added 5 commits September 13, 2024 22:30

fix test

2455119

add skipped test and todos

5cb8fa7

pr feedback

23de9d2

pr feedback

850c146

stuff

592d0fe

github-actions bot added changeset-present public api change Changes to a public API and removed public api change Changes to a public API changeset-present labels Sep 16, 2024

taylorsw04 reviewed Sep 16, 2024

View reviewed changes

packages/dds/tree/src/test/simple-tree/identifierIndex.spec.ts Outdated Show resolved Hide resolved

taylorsw04 reviewed Sep 16, 2024

View reviewed changes

remove onlys

e652c9d

taylorsw04 reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tree index api prototype #22491

feat: tree index api prototype #22491

jenn-le commented Sep 12, 2024

taylorsw04 Sep 13, 2024

jenn-le Sep 16, 2024

taylorsw04 Sep 16, 2024

taylorsw04 Sep 16, 2024

jenn-le Sep 16, 2024

taylorsw04 Sep 13, 2024

jenn-le Sep 16, 2024

taylorsw04 Sep 16, 2024

taylorsw04 Sep 16, 2024

jenn-le Sep 16, 2024

github-actions bot commented Sep 16, 2024

taylorsw04 Sep 16, 2024

taylorsw04 Sep 16, 2024

taylorsw04 Sep 16, 2024 •

edited

Loading

taylorsw04 Sep 16, 2024

taylorsw04 Sep 16, 2024

feat: tree index api prototype #22491

Are you sure you want to change the base?

feat: tree index api prototype #22491

Conversation

jenn-le commented Sep 12, 2024

Description

Reviewer Guidance

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

github-actions bot commented Sep 16, 2024

linkcheck output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylorsw04 Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylorsw04 Sep 16, 2024 •

edited

Loading