feat(weave): add view_spec_ref column to call schema #6008
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bcsherma
changed the base branch from
fix/boolean-filter-saved-view
to
graphite-base/6008
bcsherma
force-pushed
the
feat/view-spec-ref-schema
branch
from
dc9c1d9 to
e1075a7
Compare
bcsherma
force-pushed
the
graphite-base/6008
branch
from
90675b2 to
7ef926d
Compare
bcsherma
force-pushed
the
feat/view-spec-ref-schema
branch
3 times, most recently
from
c64edba to
9e92d48
Compare
Adds a new view_spec_ref column to the call schema that will store a reference to a CallViewSpec object containing view configurations. This is a schema-only change that prepares for storing call views in a dedicated column instead of the summary blob. Changes: - Add view_spec_ref field to Call dataclass - Add view_spec_ref to trace server interface schemas - Add view_spec_ref to ClickHouse and SQLite schemas - Add ClickHouse migration 024 to add the column
bcsherma
force-pushed
the
feat/view-spec-ref-schema
branch
from
9e92d48 to
3487713
Compare
|
❌ Documentation Reference Check Failed No documentation reference found in the PR description. Please add either:
This check is required for all PRs that start with "feat(weave)" unless they explicitly state "docs are not required". Please update your PR description and this check will run again automatically. |
| ADD COLUMN view_spec_ref Nullable(String) DEFAULT NULL; | ||
|
|
||
| -- Add view_spec_ref to stats table | ||
| ALTER TABLE calls_merged_stats |
There was a problem hiding this comment.
not sure we need this. worth thinking about
|
|
||
| -- Add view_spec_ref to raw call parts table | ||
| ALTER TABLE call_parts | ||
| ADD COLUMN view_spec_ref Nullable(String) DEFAULT NULL; |
There was a problem hiding this comment.
technically Nullable columns are strictly worse than non-null columns. Given that we are storing a string, would you be amenable to considering using empty string as essentially None? There are reasons to say no here, but in general we should move toward that model.
| argMaxState(display_name, call_parts.created_at) as display_name, | ||
| anySimpleState(coalesce(call_parts.started_at, call_parts.ended_at, call_parts.created_at)) as sortable_datetime, | ||
| anySimpleState(otel_dump) as otel_dump, | ||
| anySimpleState(view_spec_ref) as view_spec_ref |
There was a problem hiding this comment.
if we ever want to update this ref, we will need this to be an argmax
There was a problem hiding this comment.
not sure if that even makes sense given this object, but just a thought.
|
Consolidated into #6019 |
Summary
Adds a new
view_spec_refcolumn to the call schema that will store a reference to aCallViewSpecobject containing view configurations. This is a schema-only change that prepares for storing call views in a dedicated column instead of the summary blob.Changes
view_spec_reffield toCalldataclassview_spec_refto trace server interface schemas (CallSchema,EndedCallSchemaForInsert,CompletedCallSchemaForInsert)view_spec_refto ClickHouse schema (CallEndCHInsertable,CallCompleteCHInsertable,CallSelectCHResult)view_spec_refto SQLite schemaview_spec_reftoALLOWED_CALL_FIELDSin query buildercall_parts,calls_merged,calls_complete, and stats tables