Code Review

Overview

Clean simplification — collapsing the `json_each(row_to_json(...)) WITH ORDINALITY` pattern into a single `jsonb_build_array(sub.*)` wrapper is a clear improvement, and reusing `executeArbitraryQuery` inside `executeSecuredQuery` removes real duplication.

Issues

1. `selectChildSpansAndLogs` is not migrated (line 362)

This function still uses the old `json_build_array(r)::jsonb` pattern with explicitly-enumerated columns — the only remaining call site with the old approach. The PR description claims uniformity across all four call sites, but there are actually five active query sites.

Even if full migration is not in scope here, the explicit `json_build_array(...)::jsonb` cast is redundant — `jsonb_build_array` is the native form and avoids the intermediate json→jsonb cast:

```haskell
-- Current
rawSql ("SELECT json_build_array(" <> r <> ")::jsonb FROM otel_logs_and_spans ...")

-- Minimal fix (drop the cast, align with new pattern)
rawSql ("SELECT jsonb_build_array(" <> r <> ") FROM otel_logs_and_spans ...")
```

2. Single-use `rawCols` binding in `fetchEventExamples`

The intermediate `rawCols = colsNoAsClause cols` is used exactly once. Inline it:

```haskell
processedCols = map (\c -> if c == "summary" || "summary" `T.isSuffixOf` c then "to_json(summary)" else c) (colsNoAsClause cols)
```

3. Raw `show limit` in SQL string in `executeSecuredQuery`

```haskell
let limited = "SELECT * FROM (" <> userQuery <> ") AS subq LIMIT " <> show limit
```

`limit` is a Haskell `Int` so there is no injection risk. But the rest of this module uses Hasql interpolation for literals (e.g. `#{skip}::BIGINT` in `fetchEventExamples`). Consider staying consistent:

```haskell
let limited = rawSql ("SELECT * FROM (" <> userQuery <> ") AS subq") <> [HI.sql| LIMIT #{limit}|]
```

Minor

• The `RawSessionRow` doc update correctly removes the stale "bug workaround" rationale.
• `executeSecuredQuery` now produces two nested subquery levels (`subq` inside `sub`), same as the old code. No regression, but worth confirming both backends handle it cleanly.

Summary

Core change is correct and the direction is right. Actionable items: align `selectChildSpansAndLogs` to `jsonb_build_array` (eliminates the redundant json→jsonb cast), inline `rawCols`, and use Hasql interpolation for the `LIMIT` value.

Code Review

Overall: Clean simplification — collapsing four call-sites onto one wrapper shape is a good move, and jsonb_build_array(sub.*) is both shorter and more correct (no JSON parser issues with NULL bytes/surrogates). A couple of things to tighten:

Dead code — valueToVector should be deleted

-- LogQueries.hs:402-405
valueToVector :: AE.Value -> Maybe (V.Vector AE.Value)
valueToVector val = case val of
  AE.Array arr -> Just arr
  _ -> Nothing

This was the only call site:

-- old selectChildSpansAndLogs
pure $ keepDescendantsOf ... (mapMaybe valueToVector results)

The PR replaces it with V.toList results (correct, since executeArbitraryQuery now returns V.Vector (V.Vector AE.Value) directly), but the definition wasn't removed. It's now unreachable — delete it.

As a bonus, valueToVector and jsonArrayToVector are identical functions (both AE.Array arr -> Just arr; _ -> Nothing). Removing the dead one eliminates the duplication entirely.

Unnecessary binding — rawCols in fetchEventExamples

      rawCols = colsNoAsClause cols           -- bound here
      processedCols = map (\c -> ...) rawCols -- used exactly once

rawCols is bound and referenced only once. Given the project's line-count priority, inline it:

      processedCols = map (\c -> if c == "summary" || "summary" `T.isSuffixOf` c then "to_json(summary)" else c) (colsNoAsClause cols)

Drop the intermediary binding.

Minor nits

• selectChildSpansAndLogs: The conversion from mapMaybe valueToVector results to V.toList results is correct. keepDescendantsOf takes [V.Vector AE.Value] and executeArbitraryQuery returns V.Vector (V.Vector AE.Value), so V.toList is the right unwrap.
• executeSecuredQuery: show limit interpolated into SQL is safe (limit :: Int), no injection risk.
• The PR description accurately matches the diff — appreciated.

Summary: LGTM modulo the dead valueToVector (should be deleted before merge) and the one-use rawCols binding (minor, but goes against the stated line-count goal).

Code Review

Overview

Clean consolidation that replaces the json_each(row_to_json(sub.*)) WITH ORDINALITY two-step with a single jsonb_build_array(sub.*) wrapper across all four call sites. The PR correctly removes the duplicate valueToVector (which was identical to the already-present jsonArrayToVector), and refactors executeSecuredQuery and selectChildSpansAndLogs to delegate to executeArbitraryQuery rather than re-implementing the wrapping.

Issues / Suggestions

1. Unnecessary V.toList roundtrip in selectChildSpansAndLogs (line 371)

executeArbitraryQuery returns V.Vector (V.Vector AE.Value), but keepDescendantsOf still takes [V.Vector AE.Value], so you immediately convert back:

pure $ keepDescendantsOf ... seedSpanIds (V.toList results)

At 2000 rows this is negligible, but if keepDescendantsOf's signature were widened to Foldable t => (or changed to take a V.Vector), the allocation would disappear entirely and the call site becomes results directly. Worth a follow-up.

2. $ → () change in fetchEventExamples (line ~837) is inconsistent

-- before
processedCols = map (\c -> ...) $ colsNoAsClause cols
-- after
processedCols = map (\c -> ...) (colsNoAsClause cols)

Both are valid Haskell, but the surrounding codebase (and this file) consistently uses $ for trailing arguments. The parenthesised form doesn't buy anything here and adds a small inconsistency. Suggest reverting.

3. Silent discard in executeArbitraryQuery

pure $ V.fromList $ mapMaybe jsonArrayToVector results

A non-Array value from jsonb_build_array is silently dropped. In practice this should never happen, but it masks deserialization surprises. A logWarn or a traceEvent on unexpected values would make the failure mode observable. (Pre-existing pattern, so fine to leave as a follow-up, but worth noting since the wrapper changed.)

4. show limit in executeSecuredQuery (line 219)

let limited = "SELECT * FROM (" <> userQuery <> ") AS subq LIMIT " <> show limit

With Relude show :: Show a => a -> Text this is fine for Int. Not a bug, but worth calling out for reviewers unfamiliar with the Relude override — a tshow or explicit T.pack (Prelude.show limit) would make the intent unambiguous.

What's good

• Deleting valueToVector is the right call — it was a byte-for-byte duplicate of jsonArrayToVector.
• executeSecuredQuery now has one fewer copy of the wrapping query; the delegation to executeArbitraryQuery is correct and simplifies the exception-handling surface.
• The updated comment on RawSessionRow accurately reflects that the "NULL-bytes workaround" rationale is gone (since jsonb_build_array handles them natively), keeping the comment honest.
• The double-subquery added by executeArbitraryQuery in selectChildSpansAndLogs is harmless — the LIMIT 2000 lives in the inner query and the planner will flatten the outer select.

Test coverage / risk

The manual test against a dev TimeFusion cluster with TF PR #49 is still pending per the test plan. This is the main risk gate before merging. The PG path looks safe — jsonb_build_array(sub.*) is native in PG 12+.

Code Review (additive — building on prior comments)

The core change is correct and the direction is right. A few points not covered or needing correction in the prior reviews:

Correction: valueToVector is deleted in this PR

Review #2 claims the valueToVector definition was not removed. It is — the diff's - lines show all three lines of valueToVector deleted. No action needed here.

keepDescendantsOf — widen to Foldable instead of V.toList at call site

-- call site (line 371)
pure $ keepDescendantsOf ... seedSpanIds (V.toList results)

-- signature (line 383)
keepDescendantsOf :: HM.HashMap Text Int -> V.Vector Text -> [V.Vector AE.Value] -> [V.Vector AE.Value]

The V.toList is forced by the [V.Vector AE.Value] parameter, allocating a new list from the Vector. Since the list comprehension inside keepDescendantsOf already iterates rows once, changing the signature to accept Vector directly eliminates the allocation:

keepDescendantsOf :: HM.HashMap Text Int -> V.Vector Text -> V.Vector (V.Vector AE.Value) -> [V.Vector AE.Value]
keepDescendantsOf colIdxMap seedSpanIds rows
  | V.null seedSpanIds || V.null rows = []
  | otherwise =
      let childrenByParent = HM.fromListWith (<>) [(p, [r]) | r <- V.toList rows, Just p <- [pid' r]]
          ...

Call site becomes just results with no conversion. The null rows guard also upgrades to V.null rows (avoiding the Foldable→list coercion GHC inserts for null on a Vector).

OverloadedLists already in scope — drop V.fromList qualifier

OverloadedLists is a default-extension in this library. Line 207:

-- current
pure $ V.fromList $ mapMaybe jsonArrayToVector results

-- with OverloadedLists (already enabled)
pure $ fromList $ mapMaybe jsonArrayToVector results

The type context (Eff es (V.Vector ...)) resolves fromList to V.fromList unambiguously. Saves the V. qualifier, consistent with how OverloadedLists is used elsewhere in the codebase.

show limit — Relude's show :: Show a => a -> Text is fine here

No injection risk, and with NoImplicitPrelude + Relude, show limit already returns Text. This is idiomatic for this codebase.

Summary: Correct change overall. The two actionable items: (1) change keepDescendantsOf's third argument from [V.Vector AE.Value] to V.Vector (V.Vector AE.Value) to eliminate the V.toList at the call site, and (2) use fromList instead of V.fromList in executeArbitraryQuery (already covered by OverloadedLists).

Code Review

Overall: Clean, well-reasoned simplification. The unified jsonb_build_array(sub.*) wrapper removes real duplication and the PR description clearly explains the deployment dependency on TF #49. A few issues worth addressing.

Bug / Inaccuracy

RawSessionRow comment claims jsonb_build_array is safe for null bytes — this is wrong.

PostgreSQL's jsonb type rejects null bytes in text values regardless of which jsonb function builds the value. The old comment was accurate: row_to_json/json_each suffered from it, and jsonb_build_array does too. The RawSessionRow typed-decode path is kept for perf reasons, not safety — the comment update overstates the guarantee and could mislead future readers.

Suggested replacement:

-- Mirrors the column order of the SELECT in 'fetchSessions'. Decoded
-- directly via hasql as a perf-oriented typed path; bypassing
-- 'executeArbitraryQuery' avoids double-decoding the full row as JSON.

Inconsistency

fromList vs V.fromList in the same file.

Line 207 (new) uses bare fromList (good — leverages OverloadedLists), but lines 524, 529, 552, 574 still use V.fromList. With OverloadedLists in default-extensions, the qualified form is unnecessary noise. Either update the remaining four sites (preferred), or revert 207 back to V.fromList for consistency.

Minor

keepDescendantsOf converts V.Vector to list unnecessarily.

childrenByParent = HM.fromListWith (<>) [(p, [r]) | r <- V.toList rows, Just p <- [pid' r]]

The type signature now takes a V.Vector, but immediately calls V.toList. A small tightening avoids the intermediate allocation:

childrenByParent = V.foldl' (\m r -> maybe m (\p -> HM.insertWith (<>) p [r] m) (pid' r)) HM.empty rows

Not blocking — just worth noting given the project's priority on conciseness and avoiding unnecessary conversions.

Approved with suggestions

The core refactor is correct and the net -22 lines is real. The deployment-order documentation in the PR description is exactly right. Address the RawSessionRow comment inaccuracy before merging; the fromList inconsistency and V.toList nit can be bundled in as a follow-up.

Review

The previous review items have all been addressed — valueToVector deleted, rawCols inlined, selectChildSpansAndLogs migrated to executeArbitraryQuery, keepDescendantsOf widened to V.Vector. One outstanding item and one new one:

1. show limit — still using string concatenation (flagged in prior reviews, still open)

-- src/Models/Apis/LogQueries.hs
let limited = "SELECT * FROM (" <> userQuery <> ") AS subq LIMIT " <> show limit
resultE <- try @Hasql.HasqlException $ executeArbitraryQuery (rawSql limited)

Int can't inject anything, but the rest of this module uses Hasql interpolation for all scalar literals. Consistent form:

let limited = rawSql ("SELECT * FROM (" <> userQuery <> ") AS subq") <> [HI.sql| LIMIT #{limit}|]
resultE <- try @Hasql.HasqlException $ executeArbitraryQuery limited

This also drops the extra rawSql call on the combined Text.

2. keepDescendantsOf — V.toList rows inside a function that now accepts V.Vector

The signature correctly moved to V.Vector (V.Vector AE.Value), and V.null rows is the right guard. The body still does:

childrenByParent = HM.fromListWith (<>) [(p, [r]) | r <- V.toList rows, Just p <- [pid' r]]

V.toList materialises the vector once — unavoidable for the list comprehension. If the file ever migrates toward bare toList (Foldable), the explicit V. prefix becomes unnecessary and signals general Foldable intent rather than an explicit Vector→List coercion. Not a blocker — just worth noting if that direction is pursued.

Overall

Net −22 lines, one wrapper shape, no regression in the hasCountOver handling (the count(*) OVER() column is part of sub.* expansion, so the old +fromEnum hasCountOver offset is correctly gone), and the RawSessionRow comment now accurately describes the jsonb bypass. Good cleanup.

Code Review

Overall: Clean, well-motivated refactor. The collapse onto a single jsonb_build_array(sub.*) shape is correct and the net −22 lines is real signal reduction — not cosmetic.

Issues / Suggestions

1. keepDescendantsOf — unnecessary V.toList on the hot path

childrenByParent = HM.fromListWith (<>) [(p, [r]) | r <- V.toList rows, Just p <- [pid' r]]

rows just changed type to V.Vector but the comprehension immediately materialises it back to a list. With LIMIT 2000 this is not a bottleneck, but we're in vector / unordered-containers land already — stay there:

childrenByParent = V.foldl' (\m r -> maybe m (\p -> HM.insertWith (<>) p [r] m) (pid' r)) HM.empty rows

Avoids the intermediate allocation and is consistent with the rest of the function already using V.toList only at the leaf (seeds, V.toList seedSpanIds).

2. executeSecuredQuery — triple subquery nesting

The resulting SQL is now:

SELECT jsonb_build_array(sub.*) FROM (
  SELECT * FROM (<userQuery>) AS subq   -- added by executeSecuredQuery
  LIMIT <n>
) sub                                   -- added by executeArbitraryQuery

Three nesting levels where two would do. The outer SELECT * FROM (...) AS subq exists only to attach the LIMIT cleanly. Since executeArbitraryQuery already provides the outer subquery alias sub, you could push the limit into executeArbitraryQuery's wrapper:

-- option A: pass limit into executeArbitraryQuery and let it LIMIT before wrapping
executeArbitraryQuery :: DB es => HI.Sql -> Maybe Int -> Eff es (V.Vector (V.Vector AE.Value))

Or, simpler, keep the current two-arg split but build limited without the extra SELECT *:

let limited = querySql <> [HI.sql| LIMIT #{limit}|]

This only works if the caller-supplied query ends cleanly (no trailing semicolon, no trailing ORDER BY that would conflict). Since validateSqlQuery is already in scope, this is a safe assumption — but only if you're confident all callers don't already end with a LIMIT. If not, the subquery wrapper is the safer choice and should stay with a comment explaining why.

3. Minor — AS omitted in executeArbitraryQuery

rawSql "SELECT jsonb_build_array(sub.*) FROM (" <> querySql <> rawSql ") sub"

Every other call site in the file uses AS sub / AS subq. Both forms are valid Postgres SQL, but for consistency use AS sub.

4. rawSql (... <> userQuery <> ...) — add inline note

let limited = rawSql ("SELECT * FROM (" <> userQuery <> ") AS subq") <> [HI.sql| LIMIT #{limit}|]

userQuery :: Text is embedded as raw SQL without parameter binding. This is safe only because the two guards above call validateSqlQuery and hasProjectIdFilter first — but a future reader scanning executeSecuredQuery in isolation will see naked concatenation and reach for a diff. A one-line comment (-- safe: validated by guards above) pays for itself.

Positives

• Deleting valueToVector is the right call — jsonArrayToVector was already doing the same job.
• null rows → V.null rows is a correctness fix (not just style).
• The RawSessionRow comment rewrite is clearer than what it replaced.
• [HI.sql| LIMIT #{limit}|] for the numeric limit is correct — keeps the integer as a bound parameter rather than string-interpolated.
• PR dependency chain in the description is clear and appreciated.

Code Review

Overview

This PR unifies all four executeArbitraryQuery call sites onto a single jsonb_build_array(sub.*) wrapper, removes the now-redundant column-count parameter, fixes a silent data-loss bug in processList, and replaces a subtly broken alias-stripping implementation in Pkg.Parser.

Pkg.Parser — splitTrailingAlias

Bug fix (good): The old T.splitOn "as" would silently corrupt any column containing "as" as a substring — e.g. "timestamp" → last ["time", "tamp"] → "tamp". The new approach with breakOnEnd " as " + [alnum_] guard is strictly correct.

Case-sensitivity gap: asNeedle = " as " is lowercase-only. The sibling function stripAlias on line 170 uses " AS " (uppercase). If any user column uses "col AS alias" (uppercase), splitTrailingAlias silently returns (t, Nothing) and the alias is lost. Suggest matching case-insensitively:

-- one option
asNeedle = " as "
-- match on lowercased copy, apply offsets to original
splitTrailingAlias t = case T.breakOnEnd asNeedle (T.toLower t) of
  ...

Or unify with stripAlias (line 170) which already handles uppercase.

Succinctness nit: listToColNames case can collapse to a one-liner using fromMaybe:

listToColNames = map \x -> let (e, a) = splitTrailingAlias x in fromMaybe e a

Models/Apis/LogQueries.hs

valueToVector removal: Good dedup — it was an exact duplicate of the already-present jsonArrayToVector.

keepDescendantsOf signature change: Changing rows from [V.Vector AE.Value] to V.Vector (V.Vector AE.Value) removes the mapMaybe valueToVector call at the call-site (the unwrapping is now inside executeArbitraryQuery). The V.toList rows inside the list comprehension is O(n) and was always going to happen — no regression.

executeSecuredQuery — LIMIT injection: rawSql ("SELECT * FROM (" <> userQuery <> ") AS subq") embeds userQuery as raw SQL (validated upstream — not a regression). The [HI.sql| LIMIT #{limit}|] parameterises limit correctly.

Minor: the outer SELECT * FROM (...) AS subq wrapper in executeSecuredQuery produces a 3-layer nest after executeArbitraryQuery adds its own SELECT jsonb_build_array(sub.*) FROM (SELECT * FROM (...) AS subq LIMIT N) sub. The extra SELECT * layer is harmless for PG/TF but could be collapsed — low priority.

Opentelemetry/OtlpServer.hs — onException removal

Correctness fix, well-motivated. The handleException wrapper silently returned ack-IDs for a failed write, causing the broker to commit offsets and permanently drop the batch. Removing it lets Pkg.Queue's batch-failure handler do the right thing (transient → redeliver, permanent → DLQ).

The explanatory comments at both the removed call-site and processBatchPipeline are helpful for reviewers who might be tempted to re-add the catch.

TimefusionWriteFailureSpec.hs

Good regression spec — covers the four relevant combinations (TF up/down × good/corrupt msgs). The "KNOWN GAP" test that documents the pre-existing decode-failure ack-instead-of-DLQ behaviour is a nice touch.

One minor: try @_ @SomeException works but the wildcard on the monad is unusual; try @SomeException (with MonadUnliftIO constraint propagating) would be cleaner if the UnliftIO.Exception import allows it.

Summary

The case-sensitivity gap in splitTrailingAlias is the only item I'd want resolved before merge — everything else is improvements.

Found 9 test failures on Blacksmith runners:

Failures

Code Review

Overall this is a well-structured refactor with a clear correctness win (silent-ack-on-failure → Either WriteFailure) and good test coverage. A few spots worth addressing before merge.

🐛 Partial / unsafe

fromJust in combinePoison (Telemetry.hs):

row = maybe (fst (fromJust tfEntry)) fst pgEntry

The invariant (if pgEntry is Nothing, tfEntry must be Just) holds because rid came from HM.union, but the dependency on fromJust is invisible at the call-site and GHC won't warn. Since these is already imported, the simplest safe replacement is:

row = fst $ fromJust (pgEntry <|> tfEntry)

Or restructure with Data.Semialign.align (see below) and eliminate fromJust entirely. Either way, drop the import Data.Maybe (fromJust).

Partial pattern in routeBatchOutcome (Queue.hs):

firstReason = case poison of (_, _, r) : _ -> r

This is structurally safe (the [] branch is already handled by the guard above), but it's non-exhaustive and HLint will flag it. Use listToMaybe:

firstReason = maybe "" (\(_, _, r) -> r) (listToMaybe poison)

✂️ Redundant code

rowPoisonInfo is exported but is literally just an alias for the RowPoisonInfo constructor (which is already exported via RowPoisonInfo (..)):

rowPoisonInfo :: Maybe SomeException -> Maybe SomeException -> RowPoisonInfo
rowPoisonInfo = RowPoisonInfo   -- identical to constructor

Delete this function. Callers should use RowPoisonInfo directly.

🔧 combinePoison — leverage these / simplify

these is already a dependency and Data.These.Combinators is imported in tests. combinePoison builds two () <$ shadow-maps just to enumerate a union, then does two separate lookups + a fromJust. The same operation falls out naturally with HM.intersectionWith + HM.difference:

combinePoison :: BulkInsertResult -> BulkInsertResult -> V.Vector (OtelLogsAndSpans, RowPoisonInfo)
combinePoison pg tf =
  let mkMap bir = HM.fromList [(r.id, (r, e)) | (r, e) <- V.toList bir.poisonRows]
      pgMap = mkMap pg
      tfMap = mkMap tf
      both   = HM.intersectionWith (\(r, pe) (_, te) -> (r, RowPoisonInfo (Just pe) (Just te))) pgMap tfMap
      pgOnly = HM.map (\(r, e) -> (r, RowPoisonInfo (Just e) Nothing)) (pgMap `HM.difference` tfMap)
      tfOnly = HM.map (\(r, e) -> (r, RowPoisonInfo Nothing (Just e))) (tfMap `HM.difference` pgMap)
   in V.fromList $ HM.elems (both <> pgOnly <> tfOnly)

No fromJust, no () <$, clearer. If semialign/Data.Semialign is available (it ships alongside these in recent stackage snapshots), align pgMap tfMap :: HM.HashMap Text (These (...) (...)) makes it even more concise.

💡 Minor observations

• splitTrailingAlias (Pkg.Parser.hs) uses T.breakOnEnd which returns the last " as " — correct behavior for e.g. x as a as b → ("x as a", "b"). Worth a doctest covering that edge case since listToColNames / colsNoAsClause both delegate to it.

• retryHasqlWrite "TuplesOk" match (T.isInfixOf (show e)) is inherited from the old retryTimefusion and is fine, but the PG side (which never produces this wire tag) will still pay the show e cost on every non-transient error. Negligible in practice.

• processList — removing onException / handleException is the correct fix; the old path silently acked all messages on any exception. The regression spec in TimefusionWriteFailureSpec pins this down nicely.

• insertSystemLog explicitly void-discards the WriteFailure — the comment justifies this, but it means a broken dual-write store is invisible at the system-log level. Worth a logAttention call on Left for observability if system logs are important for incident debugging.

• Test file comment -- Regression target: the 2026-06-10 40-min outage — consider moving the date detail to the commit message or PR body rather than the module comment, since it will be stale/confusing to readers six months from now.

✅ What's good

• BulkInsertResult Semigroup/Monoid instances are clean and correct.
• writeFailureDlqHeaders using These.these is idiomatic.
• TimefusionWriteFailureSpec is comprehensive: happy path, TF-down, PG-down, both-down, mixed batches, and header checks.
• executeSecuredQuery now parameterises LIMIT #{limit} instead of interpolating it as a string — minor security hardening.
• keepDescendantsOf type fix (V.null replacing null) is correct.

Code Review: refactor(LogQueries): collapse wrappers onto jsonb_build_array(sub.*)

Overview: This PR does two largely independent things — (1) simplifies the SQL row-extraction wrapper in LogQueries, and (2) substantially refactors dual-write error handling to surface failures via Either WriteFailure instead of fire-and-forget. Both are good directions. Notes below, ordered by priority.

1. RowPoisonInfo should be These SomeException SomeException (type-safety + conciseness)

RowPoisonInfo {pgError :: Maybe SomeException, tfError :: Maybe SomeException} is structurally identical to These SomeException SomeException — and the project already imports these. The (Nothing, Nothing) branch in reasonFor is a direct smell that the type admits an invalid state:

(Nothing, Nothing) -> "row insert failed (unknown — both sides null)"

Replacing the record with:

type RowPoisonInfo = These SomeException SomeException

eliminates that impossible case, makes reasonFor total without a catch-all, and removes ~10 lines of record plumbing. combinePoison simplifies too: both, pgOnly, tfOnly maps can use These.here, These.there, These.these rather than RowPoisonInfo constructors.

2. Stale TODO in TimefusionWriteFailureSpec.hs

-- TODO(dlq-decode-failures): pre-existing gap — decode failures are
-- log-and-acked rather than DLQ'd.
it "corrupt-only batch + TF healthy → Right ([], [poison]); Pkg.Queue DLQs it"

The test immediately below it asserts that corrupt messages are in the poison list — i.e., the TODO is already resolved by this PR. Either remove the comment or update it to describe what's still outstanding.

3. Generic error reason in ProcessMessage.hs

writePoison = [ (ackId, raw, "row insert failed") | ... ]

OtlpServer.dualWriteWithPoisonMapping produces detailed reasons ("row insert failed (pg-only): " <> show pg, etc.). ProcessMessage loses the exception detail. DLQ operators will have a harder time triaging. Consider threading the exception text through, or at minimum using writeFailureSummary-equivalent.

4. retryHasqlWrite returns Right mempty on TuplesOk

When the TuplesOk wire-mismatch is detected, Right mempty is returned — meaning BulkInsertResult { rowsInserted = 0, poisonRows = [] }. The rows did land, but monitoring will see 0 inserted for that batch, which will confuse dashboards or alerting that watches rowsInserted. Consider returning a sentinel count or at least Right (BulkInsertResult unknownCount V.empty) with a dedicated unknownCount constructor. At minimum this deserves a comment at the call site in bulkInsertOtelLogsAndSpansTF.

5. Minor: null rMsgs branch in ProcessMessage.hs

if null rMsgs
  then pure (map (\(a, _, _) -> a) rMsgs, poison, Nothing)

map f [] is just []. This can be pure ([], poison, Nothing) — same result, no unnecessary traversal.

6. Minor: processList log loses timing detail

processing_ms / db_insert_ms were removed from the structured log in favour of a single outcome field. Those fields were presumably useful for latency debugging. The tradeoff is reasonable but worth noting — if those metrics are important for production alerting, they'd need another mechanism (e.g. histogram, separate span attribute).

What's good

• retryHasqlWrite unifying PG and TF retry logic is a clean deduplication; the old retryTimefusion countdown was asymmetric.
• routeBatchOutcome extraction from both pubsubService and kafkaService removes ~60 lines of duplicated logic.
• splitTrailingAlias correctly fixes the pre-existing T.splitOn "as" bug (which would have mishandled identifiers containing "as" as a substring, e.g. timestamp, cases).
• executeArbitraryQuery simplification is clear and the dependency on WildcardFnArgExpander is well-documented.
• The new test spec has thorough coverage of the failure matrix and the DLQ header contract.

Code Review — PR #423: collapse wrappers onto jsonb_build_array(sub.*)

Overall this is a well-structured, well-documented refactor. The These-based WriteFailure / RowPoisonInfo encoding is idiomatic, the retryHasqlWrite extraction removes real duplication, and surfacing poison rows to the DLQ instead of silently dropping them is the right correctness fix. A few things worth addressing before merge:

Bugs / Correctness

splitTrailingAlias is case-sensitive — " AS " won't be split

T.breakOnEnd " as " (lowercase) will miss the common SQL convention of uppercase AS. "JSONB_ARRAY_LENGTH(errors) AS errors_count" returns ("JSONB_ARRAY_LENGTH(errors) AS errors_count", Nothing) today, so colsNoAsClause leaves the alias in.

-- Fix: normalise to lowercase before breaking, then reconstruct
splitTrailingAlias (T.strip -> t) =
  let lt = T.toCaseFold t
  in case T.breakOnEnd asNeedle lt of ...
  -- or just: `t & T.toCaseFold & T.breakOnEnd " as "`
  -- and reconstruct offsets from the original `t` using the found length

Or more simply, just call T.toCaseFold on the input before the breakOnEnd and work on the case-folded string (since we're only extracting the alias, not round-tripping the expression).

Succinctness / Package Usage

V.map (\(r, _) -> (r, e)) pairs is duplicated

This pattern appears in two branches of bulkInsertOtelLogsAndSpans (V.length pairs == 1 and d <= 0). A one-liner local binding removes both:

-- before
pure (BulkInsertResult 0 (V.map (\(r, _) -> (r, e)) pairs))
-- x2

-- after — add to `go` locals
    go d pairs = tryAny (Hasql.use $ ...) >>= \case
      ...
      where asPoisonVec e = V.map ((, e) . fst)
-- then:
      pure (BulkInsertResult 0 (asPoisonVec e pairs))

String → Text → String round-trip in processBatchPipeline

let !decodePoison = [(ackId, raw, toText err) | (ackId, raw, Left err) <- decodedMsgs]
-- err :: String stored as Text
...
forM_ decodePoison \(_, raw, err) -> recordProtoError label (toString err) raw Log.logAttention
-- toString converts it back

Store as String if recordProtoError needs String, or pass err directly from the Left err match. The PoisonMsg third component (Text) is fine to keep as-is for the DLQ payload — just avoid the intermediate toText that gets immediately converted back.

writeFailureDlqHeaders — minor: (\_ _ -> ("false", "false")) → const . const

These.these f g (const . const h) is idiomatic for the two-arg no-use case, or alternatively \_ _ -> h is already fine; just noting Data.These ships no const2.

Design / Safety

WriteFailure and RowPoisonInfo are the same type alias

type WriteFailure  = These SomeException SomeException
type RowPoisonInfo = These SomeException SomeException

They're structurally identical — accidentally passing one where the other is expected compiles silently. One-line newtypes cost nothing and remove the ambiguity entirely:

newtype WriteFailure  = WriteFailure  (These SomeException SomeException) deriving (Show)
newtype RowPoisonInfo = RowPoisonInfo (These SomeException SomeException) deriving (Show)

Haddock misplacement in insertAndHandOff

  -> EW.WorkerState OtelLogsAndSpans
  -- ^ NB: returns 'Left WriteFailure' if either store failed …

The -- ^ annotates the WorkerState parameter but the note is about the function's return semantics. Move it to a top-level -- | on the function or a -- ^ returns: on the result type.

retryHasqlWrite swallows TuplesOk as Right mempty → rowsInserted = 0

The comment acknowledges this, but it means any metric/alert on BulkInsertResult.rowsInserted will silently undercount TF writes. Worth a Log.logAttention at Info or a dedicated metric bump so the undercount is observable, not just noted in source.

Minor

{-# HLINT ignore "Redundant $!" #-} is module-wide in ProcessMessage.hs

The pragma suppresses the hint everywhere in the file. HLint 3+ supports declaration-level scoping via {- HLINT ignore "Redundant $!" -} as an inline comment on the binding if your HLint version supports it. If not, document in the pragma comment that the scope is intentionally module-wide (currently the comment says "it's load-bearing here" but the suppression is file-wide).

Tests

TimefusionWriteFailureSpec is solid — tests for both-down, TF-down, PG-down, mixed batches, and the DLQ header encoding cover the key invariants. The pendingWith guard for the PG-down case (throws rather than returning Left) is correctly documented as an intentional design decision.

One gap: there's no test that verifies writeAckIds excludes per-row writePoison entries (i.e. that successAcks = filter ... writeAckIds in processBatchPipeline works end-to-end). That path is exercised indirectly via the TF-down tests but a targeted unit test for the per-row attribution in the happy-write + partial-poison case would make the invariant explicit.

Code Review — PR #423: collapse wrappers onto jsonb_build_array(sub.*)

Good overall direction. The SQL wrapper collapse is clean, and the dual-write overhaul (silent-swallow → Either WriteFailure) is a meaningful correctness improvement. Test coverage in TimefusionWriteFailureSpec is solid. A few items below.

Bugs / Correctness

WriteFailure and RowPoisonInfo are the same type

type WriteFailure   = These SomeException SomeException
type RowPoisonInfo  = These SomeException SomeException

These are structurally identical aliases. A call site can pass one where the other is expected with no compiler help. Consider newtypes:

newtype WriteFailure  = WriteFailure  (These SomeException SomeException) deriving (Show)
newtype RowPoisonInfo = RowPoisonInfo (These SomeException SomeException) deriving (Show)

Even if left as aliases, the current arrangement is a footgun.

splitTrailingAlias is case-sensitive to " as "

asNeedle = " as "

SQL aliases like SELECT x AS y (uppercase) won't be split. The old T.splitOn "as" had the same limitation so this isn't a regression, but now that there's a named helper with a docstring, it's worth either documenting or fixing with T.toLower before the breakOnEnd.

Index-alignment in dualWriteWithPoisonMapping is unenforced

-- Index-alignment assumes 'stampOrPassthrough' and 'mintOtelLogIds' preserve
-- record order and length (both are V-mapped 1:1 today).
let !idToSource = HM.fromList (zipWith (\(ackId, raw) r -> (r.id, (ackId, raw))) perRecordSource (V.toList minted))

The ID-based lookup is safe, but if either upstream function ever drops or reorders records the zipWith silently misaligns sources to IDs. A cheap guard would be:

when (length perRecordSource /= V.length minted) $
  throwIO (ErrorCall "dualWriteWithPoisonMapping: record count mismatch after stamp/mint")

Succinctness / Packages

Redundant case mWrite in ProcessMessage.hs

-- current — two destructures of mWrite
writeRes <- case mWrite of
  Nothing -> pure (Right V.empty)
  Just (projectCaches, paired) -> ...

let pairedSpans = case mWrite of Just (_, p) -> p; Nothing -> V.empty

The second case can be:

let pairedSpans = maybe V.empty snd mWrite

V.map (\(r, _) -> (r, e)) pairs appears twice in bulkInsertOtelLogsAndSpans. With TupleSections (already enabled via GHC2024) this can be:

V.map ((, e) . fst) pairs

HM.elems (both <> pgOnly <> tfOnly) in combinePoison — since the three maps have provably disjoint keys (from intersectionWith/difference), HM.unions [both, pgOnly, tfOnly] reads more clearly (union of disjoint sets rather than monoid append).

retryHasqlWrite always called with 10 in two places. A top-level constant maxWriteAttempts = 10 would make the coupling explicit.

Minor

• processReplayEvents always returns Right (…) — the Left WriteFailure arm is dead code. The interface consistency with processList is reasonable, but a comment like -- replay never dual-writes; Left is structurally absent on the return type would prevent a future reader from wondering if they missed a failure path.

• The {-# OPTIONS_GHC -Wno-unrecognised-pragmas #-} in ProcessMessage.hs silences all unrecognised-pragma warnings in the file, not just the HLINT one. Fine as-is given the explanation comment, but worth knowing if other unrecognised pragmas sneak in later.

• combinePoison result order is non-deterministic (HashMap). This is fine for DLQ routing (each entry is processed independently) but worth one line of doc so nobody adds an order-sensitive assertion against it.

What's good

• splitTrailingAlias correctly handles internal AS (e.g. CAST(x AS VARCHAR)) which the old T.splitOn "as" broke.
• traverseToSnd otelRowSnippet replacing \r -> (r,) <$> otelRowSnippet r — good use of Relude.Extra.Tuple.
• These.these for writeFailureSummary / writeFailureDlqHeaders is clean.
• BulkInsertResult Semigroup/Monoid + mconcat replacing sum is the right shape.
• The TuplesOk swallow is preserved and now lives in the shared retryHasqlWrite helper rather than duplicated.
• TimefusionWriteFailureSpec covers the key contracts well (TF-down, PG-down-throws, mixed batch, DLQ headers).

Code Review

Overview

This PR refactors the dual-write pipeline to surface failures explicitly via Either WriteFailure rather than throwing or silently swallowing them. Key changes: bulkInsertOtelLogsAndSpans now returns per-row poison via BulkInsertResult, processList/processMessages/processReplayEvents return Either WriteFailure ([Text], [PoisonMsg]), and Pkg.Queue routes outcomes via the new routeBatchOutcome. Good net reduction in lines and a real correctness win for DLQ routing.

Issues / Concerns

1. Duplicate type aliases for the same type

type WriteFailure  = These SomeException SomeException  -- batch-level
type RowPoisonInfo = These SomeException SomeException  -- row-level

Identical definitions; GHC won't prevent mixing them up. A newtype or even type RowPoisonInfo = WriteFailure would at least document the relationship. The these package is already a dependency.

2. Misplaced/duplicated docstring on retryHasqlWrite

The -- | Retry a Hasql write... block spans past the function into maxWriteAttempts, ending with a second -- | that starts a new Haddock comment but visually reads as part of the previous one. maxWriteAttempts should have its own standalone doc comment placed above it, not embedded inside the function's docstring.

3. combinePoison re-implements Data.Align — the project now depends on these

The three-way HashMap join (pgOnly / tfOnly / both) is exactly what Data.Align.align from the these-lens/semialign companion (shipped with these) is for. At minimum worth a note that this was considered; at best a one-liner:

import Data.Semialign (align)
-- align :: (Align f) => f a -> f b -> f (These a b)

Even if semialign isn't in scope yet, HM.intersectionWith + HM.difference + HM.union is fine, but the current pattern duplicates what the package provides.

4. Silent drop risk in dualWriteWithPoisonMapping and processMessages

, Just (ackId, raw) <- [HM.lookup r.id idToSource]

If the lookup ever misses (the length-equality guard proves it can't today), the row is silently absent from both the ack list and the poison list — neither written nor DLQ'd. A fromMaybe (error "invariant: …") or a logAttention-then-skip would make the invariant explicit and fail loud rather than silently drop data.

5. colsNoAsClause behaviour change: old used mapMaybe/headMay, new uses map/fst

For unaliased columns the result is the same. Worth adding a doctest:

-- >>> colsNoAsClause ["id", "CAST(x AS INT)"]
-- ["id","CAST(x AS INT)"]

to guard against future regressions on the CAST(x AS TYPE) case that motivated splitTrailingAlias.

6. BulkInsertResult derives Generic but nothing uses it

No FromJSON/ToJSON/NFData etc. is derived from it. The deriving stock (Generic) can be dropped.

7. processReplayEvents always returns Right

The comment acknowledges this, but returning Either WriteFailure … for a function that provably never returns Left means the signature over-promises. A -- invariant: always Right; replay has no dual-write note on the type (or a narrower type alias) would help the next reader.

Minor style / cleanup (all good)

• fetchRawForMerge: first firstEventTimestamp — clean.
• tolerantLogger, Web/I18n.t, Anomalies eta-reductions, Pkg/Mail guard chains — all idiomatic.
• Pages/Settings.hs addDays (-1) over pred — more explicit, correct.
• splitTrailingAlias T.breakOnEnd approach correctly handles CAST(x AS VARCHAR) vs trailing as — real bug fix.

Test coverage

TimefusionWriteFailureSpec covers the key paths well. One suggestion: the "PG down → throws" test documents a surprising invariant (lookup failure precedes write failure, so the contract is throw not Left). A short comment explaining why the Either contract doesn't hold here would help the next reader expecting uniform Left on any infrastructure failure.

Summary

Solid refactoring with a real correctness improvement — no more silent acks of DLQ-worthy messages, and POISON_ROW_DROPPED becoming a surfaced Left instead of a silent loss is a meaningful change. Main asks: unify or newtype WriteFailure/RowPoisonInfo, fix the doubled docstring on retryHasqlWrite, drop unused Generic, and add a defensive note on the HM.lookup invariant sites.

Code Review — PR #423: collapse wrappers onto jsonb_build_array(sub.*)

A well-motivated refactor with a clear net-negative line count. The dual-write outcome lift (unit → Either) is the right call and the these package is used cleanly throughout. A few things worth addressing:

Bugs / correctness

splitTrailingAlias is case-sensitive to " as " (Pkg/Parser.hs):

asNeedle = " as "

SQL AS is case-insensitive; a column alias written expr AS alias will silently fall through to the (t, Nothing) branch and appear in the SELECT instead of just the alias. The old T.splitOn "as" had the same bug, but colsNoAsClause and listToColNames now both delegate here so the blast radius is wider. Recommend T.toLower t before the breakOnEnd (keeping the original case for the expression side).

gRPC paths silently lose per-row poison (OtlpServer.hs:throwOnWriteFailure):

Right rowPoison
  | not (V.null rowPoison) ->
      Log.logAttention "OTLP_GRPC_ROW_POISON_BUT_RETURNING_OK" ...

processLogsRequest/processTraceRequest log the poison but don't DLQ it — the raw bytes are gone. The gRPC callers don't have ack-ids to route to routeBatchOutcome, so this is a structural gap, but it should at least be a visible TODO rather than a silent log.

Type safety

WriteFailure and RowPoisonInfo are the same type alias (Telemetry.hs):

type WriteFailure  = These SomeException SomeException
type RowPoisonInfo = These SomeException SomeException

Semantically different (batch-level vs row-level) but the compiler can't enforce the distinction. Newtype wrappers around each would prevent accidental transposition at call sites — especially important now that both are threaded through insertAndHandOff's return type.

Haddock ordering

The long haddock for retryHasqlWrite is immediately followed by a separate -- | for maxWriteAttempts, which sits before the function it documents. The doc visually bleeds:

-- | Retry a Hasql write up to @n@ times ...
-- ...
-- | Cap on retry attempts ...    ← starts a NEW haddock, not a continuation
maxWriteAttempts :: Int

retryHasqlWrite ...  ← its actual haddock is above maxWriteAttempts

Move maxWriteAttempts above the retryHasqlWrite haddock block, or place its -- | directly on the function below.

Minor succinctness

• routeBatchOutcome: map fst validMsgs → fst <$> validMsgs; toText (show e) → show @Text e (consistent with the rest of the file).
• insertSlice: V.map (\(r, _) -> (r, e)) pairs appears twice (single-row poison + bisect-exhausted poison); a local toPoison e = V.map (\(r, _) -> (r, e)) would DRY it.

Behaviour asymmetry worth documenting

The test file captures this, but it's non-obvious at the API level:

• TF down → processList returns Left (That tfErr)
• PG down → processList throws (project-key lookup hits PG before any write attempt)

routeBatchOutcome handles both (the outer try wraps), but a one-line note on insertAndHandOff's return type saying "PG infrastructure failures throw rather than Left" would save the next reader from re-deriving it.

What's good

• these is exactly the right package for two-sided error attribution; These.these fold in writeFailureSummary / poisonReason is idiomatic.
• traverseToSnd / toSnd from relude-extra replacing hand-written lambdas is the right call.
• combinePoison via HashMap intersection/difference is correct and avoids ordering dependency.
• The new TimefusionWriteFailureSpec covers the invariants that matter (TF-down → Left That, PG-down → throw, mixed batch, corrupt-only). The withBrokenTf/withBrokenPg fixture pattern is clean.
• BISECT_DEPTH_EXHAUSTED now surfaces rows as poison instead of re-throwing — correct for the DLQ model.
• pred ce → addDays (-1) ce in Settings.hs is strictly more correct.