Refetch computed for newly inserted objects #869

dnwpark · 2025-08-22T18:53:46Z

Given a schema

type A {
   total_val := sum(.<a[is B].val);
};
type B {
    a: A;
    val: int64;
};

If a user does:

a = default.A()
b = default.B(a=a, val=7)

client.sync(a, b)

The order of inserts will be a, then b and so the value of a.total_val fetched during the insert will be 0.

This PR adds a refetch query for the computed properties of newly inserted objects.

Tests are based on #848

1st1 · 2025-08-25T18:40:24Z

gel/_internal/_save.py

+                ref_shape.fields[field_name] = ptr_info
+
+    else:
+        # New objects only need computeds refetched


Wait, strictly speaking this isn't true and I'm not sure why we need this "optimization". Even new object's links can be rewritten by a trigger/mutation rewrites.

So the question is why do we need to bother and should we?

I did this because the design says we should refetch all computed properties for new objects, but only set properties for existing objects.

Probably part of the intended behaviour.

Not by a trigger, but by a rewrite certainly. We probably do want to refetch all of them?

(Which design do you mean here?)

1st1 · 2025-08-25T18:41:39Z

gel/_internal/_save.py

+
+    else:
+        # New objects only need computeds refetched
+        for field_name in obj.__pydantic_computed_fields__:


I'd use pointer introspection here, not __pydantic_computed_fields__

1st1 · 2025-08-25T18:43:26Z

gel/_internal/_save.py

            )
            for obj in shape.models:
-                link_ids = obj_links_all[obj.id]
+                link_ids = obj_links_all[self._get_id(obj)]


1st1 · 2025-08-25T18:44:28Z

gel/_internal/_save.py


    def __post_init__(self) -> None:
        self.new_object_ids = {}
+        self.new_objects = {}


I've changed the collection type for new_object_ids

please rebase onto the fresh master

1st1 · 2025-08-25T18:45:36Z

gel/_internal/_save.py

                        # see the above comment.
                        multi_prop_commit_recursive(linked)

+            if new_obj_id := self.new_object_ids.get(id(obj)):


yeah, rebase is needed. Basically with the latest master you'd just self.new_object_ids.get(obj)

dnwpark · 2025-09-03T16:04:40Z

This now includes fixes for #884

msullivan · 2025-09-03T21:59:29Z

gel/_internal/_save.py

    tp_pointers = tp_obj.__gel_reflection__.pointers
    ref_shape = refetch_ops[tp_obj]
    ref_shape.models.append(obj)
+    # Existing objects should refetch anything that was previously set


Not just existing objects, right? New objects too?

msullivan · 2025-09-03T22:02:54Z

gel/_internal/_save.py

+                    if (
+                        # prop not refetched
+                        prop.name not in new_obj.__dict__
+                        # TODO: Refetching links for new objects


msullivan · 2025-09-03T22:06:12Z

gel/_internal/_save.py

+            if (
+                self.refetch
+                and obj in self.new_object_ids
+                and (new_obj_id := self.new_object_ids.get(obj))
+            ):
+                new_obj = self.new_objects.get(new_obj_id)


What's going on here? Why does new_objects need a different path for updating the objects with refetched data than the old objects?

There are 3 instances of the same object:

the user instance

the batch instance

the refetch instance

New objects are updated differently from existing objects since they don't have an id to start with.

The sequence of operations:

Client._save_impl: Batch queries are run in the client

QueryBatch.record_inserted_data: batch instances saved in new_objects and the relationship to the user instances is saved in new_object_ids.

SaveExecutor._commit:

Computed props from the batch instance are applied to the user instance

apply_refetched_data: the batch instance is updated with the refetch instance's data

_commit_recursive: the user instance is updated with the batch instance's data.

Step 3 has a lot of redundant work so I've simplified it to:

apply_refetched_data: the batch instance is updated with the refetch instance's data

Copy all refetched props to the user instance.

dnwpark requested review from 1st1 and msullivan August 22, 2025 19:40

dnwpark force-pushed the sync-tests branch from 8cbbd54 to d5c1734 Compare August 22, 2025 22:57

dnwpark force-pushed the refetch-new-computed branch from 963073a to d36e8c0 Compare August 22, 2025 22:57

dnwpark force-pushed the sync-tests branch from d5c1734 to 5ffe8c8 Compare August 22, 2025 22:58

dnwpark force-pushed the refetch-new-computed branch from d36e8c0 to b753f12 Compare August 22, 2025 22:58

dnwpark force-pushed the sync-tests branch from 5ffe8c8 to 1e8ad04 Compare August 22, 2025 23:09

dnwpark force-pushed the refetch-new-computed branch from b753f12 to a4ac639 Compare August 22, 2025 23:09

dnwpark force-pushed the sync-tests branch from 1e8ad04 to dc5ab3f Compare August 22, 2025 23:13

dnwpark force-pushed the refetch-new-computed branch from a4ac639 to 19fe9fa Compare August 22, 2025 23:14

Base automatically changed from sync-tests to master August 23, 2025 01:23

dnwpark force-pushed the refetch-new-computed branch 2 times, most recently from 309abbd to 0b99ffa Compare August 25, 2025 16:48

1st1 reviewed Aug 25, 2025

View reviewed changes

dnwpark force-pushed the refetch-new-computed branch 3 times, most recently from 86d8765 to 8eb33b0 Compare August 26, 2025 00:00

dnwpark marked this pull request as ready for review August 26, 2025 01:23

dnwpark force-pushed the refetch-new-computed branch from 8eb33b0 to c2debad Compare August 26, 2025 04:08

dnwpark marked this pull request as draft August 26, 2025 04:45

dnwpark marked this pull request as ready for review August 26, 2025 20:31

dnwpark force-pushed the refetch-new-computed branch from 929784c to 134a7a0 Compare August 27, 2025 16:27

1st1 requested a review from vpetrovykh September 2, 2025 17:36

dnwpark force-pushed the refetch-new-computed branch 3 times, most recently from acdf183 to dc41bdb Compare September 3, 2025 00:09

dnwpark force-pushed the refetch-new-computed branch 2 times, most recently from 146db4c to 2297769 Compare September 3, 2025 05:40

dnwpark mentioned this pull request Sep 3, 2025

Link with rewrite that rewrites to an insert statement is refetched as None #898

Closed

dnwpark force-pushed the refetch-new-computed branch from 2a162be to 4fcac61 Compare September 3, 2025 16:39

dnwpark mentioned this pull request Sep 3, 2025

Syncing a link with props with which is initialized to None produces AssertionError #883

Closed

msullivan approved these changes Sep 3, 2025

View reviewed changes

dnwpark added 11 commits September 3, 2025 15:20

Refetch new object computeds.

33cbf32

Remove xfail.

e08fd47

Add isolated branches.

26b900f

Use pointer introspection instead of pydantic fields.

25b583c

Add tests for computed props, rewrites and triggers.

aed8959

Fix other tests.

a725e44

Improve trigger tests.

c2448c4

Refetch all set fields for new objects too.

7675e9b

Update tests.

e0a6f02

Don't commit refetch links for now.

9b6a91f

Remove redundant 2 step approach to updating new objects.

464610e

dnwpark force-pushed the refetch-new-computed branch from 4fcac61 to 464610e Compare September 3, 2025 23:03

dnwpark merged commit 854a79c into master Sep 4, 2025
73 of 83 checks passed

dnwpark deleted the refetch-new-computed branch September 4, 2025 01:43

dnwpark mentioned this pull request Sep 4, 2025

Modifying a link to or from None causes an ISE when syncing #884

Closed

Refetch computed for newly inserted objects #869

Refetch computed for newly inserted objects #869

Uh oh!

Conversation

dnwpark commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnwpark commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnwpark Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnwpark commented Aug 22, 2025 •

edited

Loading

dnwpark Sep 3, 2025 •

edited

Loading