feat(weave): backend to allow deletion of calls #1655
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=0592f6558f46c1822cbf4f64df55b10bad635f4e |
gtarpenning
force-pushed
the
griffin/deleted-at-backend
branch
from
539080f
to
4e4e0ad
Compare
|
|
||
| # create insert payload for all descendants (and parents) | ||
| data = [(req.project_id, d, deleted_at) for d in all_descendants] | ||
| self._insert( |
There was a problem hiding this comment.
nit: while not necessary, it would be nice for this to use _insert_call instead of _insert. _insert feels risky to me because it is raw, untyped. _insert_call would require you to define a CallDeleteCHInsertable tp add to CallCHInsertable = typing.Union[CallStartCHInsertable, CallEndCHInsertable]
The reason i like this pattern more is that it enforces the expected schema of a delete. This is important because we absolutely must be sure that the insert only contains the fields "project_id", "id", "deleted_at"... which is enforced by this list right now. Why? Well the aggregation functions such as anySimpleState(op_name) will take any-non-null value... meaning the delete entries could inadvertently mess with other fields. This is more of the theoretical callout, so consider it optional.
gtarpenning
force-pushed
the
griffin/deleted-at-backend
branch
from
4e4e0ad
to
4f71407
Compare
| @@ -1058,6 +1134,19 @@ def _insert_call(self, ch_call: CallCHInsertable) -> None: | |||
| if self._flush_immediately: | |||
| self._flush_calls() | |||
|
|
|||
| def _insert_calls_delete( | |||
There was a problem hiding this comment.
this looks identical to _insert_call except that it supports a list. The column stuff should work just fine due to the Null defaults. I'd recommend just changing _insert_call to handle a list
There was a problem hiding this comment.
The issue is how to manage all_call_delete_columns vs. all_call_insert_columns which can be different. Not ~ quite ~ identical... @tssweeney
gtarpenning
force-pushed
the
griffin/deleted-at-backend
branch
2 times, most recently
from
7a366a0
to
912f4e9
Compare
| @@ -314,7 +316,7 @@ def calls_query(self, req: tsi.CallsQueryReq) -> tsi.CallsQueryRes: | |||
| order_parts.append(f"{field} {direction}") | |||
|
|
|||
| order_by_part = ", ".join(order_parts) | |||
| query += f"ORDER BY {order_by_part}" | |||
| query += f" ORDER BY {order_by_part}" | |||
There was a problem hiding this comment.
this actually fixes a bug
…nto griffin/deleted-at-backend
…nto griffin/deleted-at-backend
* add to sidebar * move image * cooler project name
moved to clean branch: #1673