feat(query-engine): branded identity columns derive branded row schemas; drop redundant declared rowSchemas - #652
Merged
Conversation
An audit of every surviving declared rowSchema against the builder-derived one found this site byte-identical — the builder derives the exact same struct from the SELECT's column types. The serviceOverviewRowSchema export stays: the compare path's period-wrapper still spreads its fields.
…e branded row schemas
Comparisons on a column now widen a branded decoded type to its primitive
(Widen<TSType>), so $.OrgId.eq(param.string("orgId")) keeps compiling when
the column's schema brands it — the type-level mirror of literalSchema.
Maple's tables.ts declares OrgId (all tables) and TraceId/SpanId (error_events
tables) via T.custom with their branded domain schemas, so derived row schemas
carry the brands. That retires the declared rowSchemas at activeOrgsBy* and
errorIssueSampleTraces; their output types now come off the derivation.
The SQL-catalog synthetic zero-value row ("" for String columns) would fail
a brand's minLength check, so the affected fixtures — and now pipe fixtures,
which gained sampleValues support — override those fields.
Makisuo
changed the title
…ing column
A tenant-column constraint like { OrgId: <a String column> } has to accept a
branded column, and CHType is invariant in its decoded type, so CHString can't
say it. Name the concept in the lib instead of hand-rolling
CHType<"String", any, any> at the consumer.
…ructural inference CI caught what a stale local tsbuildinfo hid: InferOutput's `S[K] extends Expr<infer T>` resolved T through the comparison methods' contravariant params — which are now widened for branded columns — so every branded column's derived output type collapsed to the bare primitive and the api call sites failed against their branded consumers. InferOutput now reads the expression's `_phantom` property directly, which is exact; Exclude strips only the optionality's undefined so Nullable columns keep their `| null`. The edges-rollup declared schema also has to say the brand now that the derived output carries it — declared schemas narrow, they don't contradict.
…service-overview-rowschema
main's invalid-sql tag added a caller-authored twin of the analyzer rule, and inserts deliberately keep the default caller authorship — so the syntax-shaped insert rejection takes the new tag. The api-side expectation was the one call-out that commit missed.
…rview-rowschema' into chore/drop-redundant-service-overview-rowschema
🍁 Maple PR previewWarning Preview cleanup could not be confirmed. The Alchemy teardown outcome was Final commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two steps toward retiring declared
rowSchemas in favour of builder-derived ones, from a full audit of every surviving declared site (probe incompileInnercomparing declared vs derived ASTs, all 19 production sites driven).1. Drop the one byte-identical declaration
The
service_overviewpipe's declared schema was identical to what the builder derives — deleted.serviceOverviewRowSchemastays exported for theservice_overview_compareperiod-wrapper.2. Branded identity columns (the "brands" unlock, implemented)
Widen<TSType>inexpr.ts), so$.OrgId.eq(param.string("orgId"))keeps compiling when the column's schema brands it — the type-level mirror ofliteralSchema("comparisons may accept more than the column decodes to").inListgeneralized the same way.T.custom("String", brandedSchema)already existed; new type-level + runtime tests pin the behaviour.tables.ts:OrgIdon all 37 tables, andTraceId/SpanIdon theerror_eventstables, now carry their branded domain schemas. Derived row schemas keep the brands, so the output types (ReadonlySet<OrgId>,TraceId/SpanIdin sample traces) come straight off the derivation.activeOrgsBy*(×2 call sites) anderrorIssueSampleTraces, plus their now-dead schema consts (ActiveOrgsOutputSchema,ErrorIssueSampleTracesOutputSchema— output types survive as interfaces).""for String columns) would fail a brand's minLength check, so the seven affected catalog entries carrysampleValuesoverrides — pipe fixtures gainedsampleValuessupport for this (builder fixtures already had it). A catalog-wide scan confirmed exactly those seven entries are brand-affected.Why the remaining declared rowSchemas survive (follow-up map)
workloadKind,targetType) — computed viaif_/multiIf, which type as first-typed-arm.divis blanket-nullable —X.div(1_000_000)derivesNullOreven though a non-zero literal denominator can't produce inf/nan. Typing literal-denominator division as plainCHNumberwould flipaiSessionSpans,serviceMapEdgesRollup.MaxDurationMs, and the ops-summary duration columns to fully derivable.CHNumberOrZero— rewritingif_(w > 0, a.div(w), 0)andsum/nullIfshapes asCH.ifNotFinite(a.div(b), 0)(already typed non-null) would retire the rest, but changes emitted SQL for ~10 live queries, so it's deliberately not in this PR.ServiceWorkloads.avgCpu/MemoryLimitUtilizationare declared nullable against a non-null derivation (LEFT JOIN nullability the deriver doesn't model) — deleting those would reintroduce decode failures.Test plan
lib/clickhouse-builder: typecheck (incl. new type-level tests) + 243 tests passpackages/query-engine: typecheck + full package suite, 1296 tests passpackages/query-engine-integrations: typecheck cleanapps/api: typecheck fully clean (0 errors) after building theeffect-sdkdist;ErrorsService+ErrorIssueReadModelsServicesuites pass (58 tests)sampleValues; the ClickHouse DESCRIBE e2e runs in theclickhouse-schema-e2eCI job🤖 Generated with Claude Code