-
[OPIK-5694] [BE] fix: eliminate FORMAT Values fast-path noise in batch inserts (#6572)
frostbyte_neo 发布于
2026-05-07 15:50:53 +00:00 - [OPIK-5694] [BE] fix: stop tripping FORMAT Values fast-path in trace/span/thread batch INSERTs
Per Liya Katz's investigation on OPIK-5694, the silent error counter
pollution in production ClickHouse (~system.errors codes 26 / 27 / 43 /
70) traces back to datetime function expressions being substituted into
the per-row tuple inFORMAT Values:parseDateTime64BestEffort(:start_time, 9),
if(:end_time IS NULL, NULL, parseDateTime64BestEffort(:end_time, 9)),
if(:last_updated_at IS NULL, now64(6), parseDateTime64BestEffort(...)),
...Empirical isolation against ClickHouse 25.3.6.56 (test container) and
25.3.8 (production version) confirmed: every datetime cell expressed as
a function call adds +1 to code 27 per statement; NULL substituted into
a non-nullable column adds +1 to code 70. The Values fast-path parser
only recognises canonical text literals for DateTime64 cells; any
function or::cast falls back to the slow path and counts the
assertion in system.errors.Fix: format datetime values to canonical ClickHouse text in Java
(yyyy-MM-dd HH:mm:ss.SSSSSSSSSfor precision 9,.SSSSSSfor
precision 6) and bind them as plain strings, dropping every parse /
if-wrap from the per-row tuple. The clickhouse-r2dbc driver substitutes
String values as'<canonical>'literals which the Values fast-path
recognises directly.Same canonical-literal idea applied to:
- TraceDAO.BATCH_INSERT: start_time, end_time, last_updated_at,
visibility_mode (was anif(... IS NULL, 'default', ...)wrap;
now bound as 'default' literal when null) - SpanDAO.BULK_INSERT: start_time, end_time, last_updated_at
(toDecimal128 / mapFromArrays for cost / usage left for a follow-up
per scope discussion) - TraceThreadDAO.INSERT_THREADS_SQL: created_at, last_updated_at, and
sampling_per_rule (the existing mapFromArrays(:rule_ids, :sampling)
was replaced by binding a Java Map directly — the clickhouse-r2dbc
driver natively serialises Map<UUID, Boolean> as{'uuid': true}map
literal, no SQL function needed)
New util: ClickHouseDateTimeFormat with NANOS / MICROS formatters in UTC.
Tests: regression tests in TracesResourceTest$BatchInsert and
SpansResourceTest$BatchInsert assert codes 26 / 27 / 43 / 70 deltas
against system.errors before/after batchCreateTraces /
batchCreateSpans:- Traces: all four codes assert isZero (covers BATCH_INSERT and the
side-effect trace_threads INSERT_THREADS_SQL that ingestion triggers) - Spans: code 70 asserts isZero. Codes 26 / 27 / 43 are NOT asserted on
spans because BULK_INSERT still has toDecimal128 / mapFromArrays in
non-datetime cells; eliminating those is out of scope for this PR.
Behaviour for clients is unchanged:
lastUpdatedAtstill defaults to
"now" when omitted;visibilityModestill defaults to 'default'.- fix(spans): bind BigDecimal/Map directly to drop toDecimal128 + mapFromArrays from BULK_INSERT
Same FORMAT Values fast-path issue as the rest of OPIK-5694: function
expressions in tuple cells trip ClickHouse's Values fast-path parser
and bump system.errors codes 26 / 27 / 43.In SpanDAO.BULK_INSERT the remaining offenders were:
- toDecimal128(:total_estimated_cost, 12) -- Decimal128 column
- mapFromArrays(:usage_keys, :usage_values) -- Map(String, Int32) column
The clickhouse-r2dbc driver natively serialises both types as canonical
literals: BigDecimal -> plain numeric (e.g. 0.000123), Map -> {key : value}
literal. So we just bind the values directly:- bindCost: pass BigDecimal (not .toString()) so SQL gets
0.000123
instead of'0.000123'(the quoted form also trips the fast-path,
empirically verified) - usage: assemble a Map<String, Integer> in Java and bind it; the driver
emits{'in' : 2, 'out' : 5}map literal
Other SpanDAO templates (single-row INSERT, UPDATE_SPAN_SQL) still use
toDecimal128/mapFromArrays and are intentionally untouched -- those are
INSERT...SELECT statements, not FORMAT Values, so they don't hit the
Values fast-path.Tests: SpansResourceTest now asserts all four codes (26/27/43/70) stay
at zero after batchCreateSpans, matching the traces test.- test: cover null and non-null branches of every changed field in batch insert tests
Audit of the regression tests revealed a gap: the new batch-insert tests
only exercised some null/non-null permutations of the fields this PR
touches. Beef up both tests so every branch the SQL/binding cares about
is hit:Traces test (3 rows in one batch):
- Row A: endTime=null, lastUpdatedAt=null, visibilityMode=null
- Row B: endTime!=null, lastUpdatedAt!=null, visibilityMode=DEFAULT
- Row C: endTime!=null, lastUpdatedAt!=null, visibilityMode=HIDDEN
Spans test (2 rows in one batch):
- Row A: endTime=null, lastUpdatedAt=null, usage=null, totalEstimatedCost=null
- Row B: endTime!=null, lastUpdatedAt!=null, usage={prompt:12, completion:7},
totalEstimatedCost=BigDecimal("0.000123456789")
Both tests still strictly assert codes 26 / 27 / 43 / 70 deltas == 0
against system.errors.startTime is @NotNull at the API layer for both Trace and Span, so the
null branch is unreachable through the public batch endpoint and not
worth testing here.- fix(spans): split bindCost into single-row vs BULK_INSERT variants
CI surfaced a precision regression in SpansResourceTest$CreateSpan#createAndGetCost
and $UpdateSpan#update__whenCostIsChanged after the previous commit changed
bindCost to bind BigDecimal directly.Root cause: bindCost is shared across SQL templates with different shapes.
- BULK_INSERT now writes the cell as a bare :placeholder (Decimal128(12)
column). It needs a BigDecimal binding so the driver emits an unquoted
numeric — CH parses straight into Decimal128 and is lossless. - The single-row INSERT and UPDATE templates still wrap the cell as
toDecimal128(:cost, 12). Those need a String binding so the driver wraps
with single quotes and CH parses via the Decimal-aware string path. With
a BigDecimal binding the unquoted literal toDecimal128(123.45, 12) routes
through Float64 and drops digits beyond ~17 significant figures (the test
failure mode).
Fix:
- Restore bindCost to the original String binding (.toString()) — keeps
INSERT/UPDATE precision intact. - Add bindCostForBulkInsert that binds BigDecimal directly. Used only in
the BULK_INSERT loop. Keeps that cell at +0 system.errors increments.
Also added a brief section to the ClickHouse skill documenting the rule:
in FORMAT Values templates, every cell must be a plain placeholder bound
to a value the driver serialises as a canonical literal — function calls
in cells flood system.errors / pod stderr even when the insert itself
succeeds.- refactor: inline BULK_INSERT cost binding, trim ClickHouse skill section
- Inline the BigDecimal-direct cost bind at the BULK_INSERT call site (only
call site for that variant), drop the bindCostForBulkInsert helper and
the docstring on the existing bindCost — both unnecessary for one
callsite. - Tighten the ClickHouse skill addition to drop the redundant single-row
INSERT/UPDATE caveat (the rule scope is already in the heading) and
dedupe overlapping language.
- fix(batch): capture now() once per batch for null lastUpdatedAt rows
CI surfaced ProjectsResourceTest$FindProject#getProjects__whenProjectsHasTracesBatch__thenReturnProjectWithLastUpdatedTraceAt
failing with a sub-millisecond timestamp diff between getById of the last
trace and the project's MAX(last_updated_at). Reproduces under CI load
but not deterministically in isolation.Root cause: the prior server-side wrap was if(:last_updated_at IS NULL,
now64(6), parseDateTime64BestEffort(...)). ClickHouse evaluates now64() once
per query — every batch row with null lastUpdatedAt got the SAME timestamp,
makinggetById(any).lastUpdatedAt == MAX(...)trivially true. After the
canonical-literal refactor we were calling Instant.now() per row in the bind
loop, so each null-lastUpdatedAt row got a distinct sequential timestamp.
WhilegetById(traces.getLast())should still equal MAX in the steady case,
JVM scheduling jitter under CI load made the assertion fragile.Fix: capture Instant.now() once per batch (outside the bind loop) and reuse
it for every row whose client did not provide lastUpdatedAt. Restores the
prior single-timestamp-per-batch invariant the test relies on.Same change in SpanDAO. TraceThreadDAO doesn't need it — that DAO never
falls back to Instant.now(); it always uses item.lastUpdatedAt() / item.createdAt().- fix(threads): bind scored_at as canonical ClickHouse text everywhere
Review feedback on PR #6572:
The scored_at cell in INSERT_THREADS_SQL is a real fast-path concern that
slipped through the original audit. INSERT_THREADS_SQL is a FORMAT Values
template and binds scored_at as a bare :placeholder (no parseDateTime64BestEffort
wrap). Until now we passed Instant.toString() (ISO-8601 with T/Z), which the
new ClickHouseDateTimeFormat util explicitly warns against — when scoredAt is
non-null (re-insert after scoring), CH would not recognise the literal and
would trip codes 26/27/43 the same way the rest of the PR addresses.Fix line 335 to use ClickHouseDateTimeFormat.formatNanos(item.scoredAt()).
Also applied the same canonical-text format at the two other scored_at bind
sites (UPDATE_THREAD_SQL line 516 and UPDATE_THREAD_SCORED_AT line 566) — those
are INSERT...SELECT / UPDATE statements wrapped in parseDateTime64BestEffort
so they were not affected by the fast-path issue, but consistent canonical
binding is zero-cost and avoids someone copy-pasting the ISO-8601 form into
a future FORMAT Values template.Aligned bindNull("scored_at", Instant.class) → String.class with the rest of
the nullable-DateTime cells in this PR.- refactor(spans): single bindCost helper, drop toDecimal128 wraps everywhere
Per Andres's review feedback on PR #6572:
- Drop toDecimal128(:total_estimated_cost, 12) from all four SQL templates
(single-row INSERT, UPDATE_SPAN_SQL, two SELECT-style templates). Bare
:total_estimated_cost lets the unquoted BigDecimal literal go straight
into the Decimal128(12) column without the Float64 detour that made the
earlier "two helpers" split necessary. - Restore a single bindCost helper that binds BigDecimal directly. No more
String/.toString() form (which was triggering the FORMAT Values fast-path
fallback when used in BULK_INSERT) and no more separate bindCostForBulkInsert. - Add @NonNull to ClickHouseDateTimeFormat.formatNanos / formatMicros for
defensive null-arg handling.
Empirically verified via curl that CH accepts plain numeric literals (0,
0.000123, 434852338.34043884, 1.23E-11, 1.23E+10, 0E-11) into Decimal128(12)
columns in both INSERT...SELECT and FORMAT Values contexts without
precision drift.- Revert "refactor(spans): single bindCost helper, drop toDecimal128 wraps everywhere"
This reverts commit 315dea4085 except for the @NonNull addition on
ClickHouseDateTimeFormat, which is kept.CI surfaced the constraint: the SELECT-style INSERT templates use
multiIf(old_span.total_estimated_cost > 0, old_span.total_estimated_cost,
new_span.total_estimated_cost) to fall back between existing-row and new-row
values. With the toDecimal128(...) wrap removed, new_span.total_estimated_cost
is typed as Float64 (CH's default for unquoted decimal literals), and CH
refuses the multiIf with "NO_COMMON_TYPE: Decimal(38, 12), Float64 because
some of them have no lossless conversion to Decimal".The single-helper / drop-the-wrap approach therefore can't satisfy three
constraints at once:- precision (no Float64 detour),
- clean fast-path for FORMAT Values (no toDecimal128 wrap, no quoted decimal),
- correct type matching in SELECT-style multiIf.
Going back to the two-helper split: bindCost (String -> toDecimal128('str', 12)
in SELECT templates -> string-direct, lossless, Decimal128-typed result column)
and bindCostForBulkInsert (BigDecimal -> bare placeholder in FORMAT Values cell
-> direct write into Decimal128 column, no fast-path fallback).Will reply on the PR thread with this evidence.
下载附件