perf(clickhouse): prepare task event attribute inserts - #14
Conversation
Source PR: triggerdotdev#4860 Source head: 113b0d6
|
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| // V2 Table Functions (partitioned by inserted_at instead of start_time) | ||
| // ============================================================================ | ||
|
|
||
| const TASK_EVENT_V2_INSERT_COLUMNS = [ |
There was a problem hiding this comment.
Shipwright · HIGH
The new TASK_EVENT_V2_INSERT_COLUMNS constant is defined as a bare array with a satisfies tuple constraint, but there is no comment explaining why explicit columns are required or
Impact: The new TASK_EVENT_V2_INSERT_COLUMNS constant is defined as a bare array with a satisfies tuple constraint, but there is no comment explaining why explicit columns are required or what happens if the table schema changes. A future maintainer adding a column to TaskEventV2Input may not realize they must also update this list, causing silent data loss or insert failures.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| expect(readError).toBeNull(); | ||
| expect(rows).toEqual([ | ||
| { | ||
| attributes_text: '{"a":"hello","nested":{"enabled":true},"z":1}', |
There was a problem hiding this comment.
Shipwright · HIGH
The test asserts an exact JSON string for attributes_text, but JavaScript object key ordering is not guaranteed.
Impact: The test asserts an exact JSON string for attributes_text, but JavaScript object key ordering is not guaranteed. The input object { z: 1, a: 'hello', nested: { enabled: true } } may serialize in a different order across runtimes or V8 versions, causing intermittent CI failures. The test should parse attributes_text and compare objects.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| columns: [ | ||
| { name: "model", expression: "attributes.gen_ai.response.model.:String" }, | ||
| { name: "system", expression: "attributes.gen_ai.system.:String" }, | ||
| { |
There was a problem hiding this comment.
Shipwright · HIGH
The migration from ClickHouse map accessor syntax (attributes.gen_ai.response.model.:String) to JSONExtractString(attributes_text, ...) changes query semantics.
Impact: The migration from ClickHouse map accessor syntax (attributes.gen_ai.response.model.:String) to JSONExtractString(attributes_text, ...) changes query semantics. If attributes_text is not populated for older rows or is populated asynchronously, these queries may silently return empty results, causing the missing-model detection to skip spans and potentially under-bill or misreport LLM usage. No backfill or data migra…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| expect(readError).toBeNull(); | ||
| expect(rows).toEqual([ | ||
| { | ||
| attributes_text: '{"a":"hello","nested":{"enabled":true},"z":1}', |
There was a problem hiding this comment.
Shipwright · MEDIUM
The test expects attributes_text to be '{"a":"hello","nested":{"enabled":true},"z":1}', but the input attributes object is { z: 1, a: "hello", nested: { enabled: true } }.
Impact: The test expects attributes_text to be '{"a":"hello","nested":{"enabled":true},"z":1}', but the input attributes object is { z: 1, a: "hello", nested: { enabled: true } }. JSON object key ordering is not guaranteed by the JavaScript runtime, so this assertion can fail intermittently depending on insertion order. The test should parse the JSON and compare objects rather than asserting an exact string.
Suggested fix: Fix the review finding before release.
1 participant
Summary
Prepare task event writes for a later storage schema change without changing the table itself. Event attributes continue to be stored and read exactly as before.
Design
The event writer now names every insert column explicitly, ensuring
attributesremains part of the input when the column becomes input-only. The remaining native attribute-path queries read the existingattributes_textrepresentation through JSON extraction functions.This PR does not alter the
task_events_v2schema.Source merge-base:
43ecf15f80277c0eb931c08ddd89de55179cc795Source head:
113b0d66f77c81717f28bae50f150227bcc79652