Code Review

Overview

This PR fixes a correctness bug in trace-tree expansion: previously selectChildSpansAndLogs returned all spans in a matching trace (siblings, uncles, unrelated roots), not just descendants of the matched spans. The fix adds a BFS walk in memory after a bounded SQL fetch, gated by a new withChildren flag that defaults to false for the API/CLI and true for the UI.

The logic is sound and the test coverage is solid. A few issues worth addressing:

Issues

1. Unnecessary Data.Map.Strict import — use HM.HashMap instead

keepDescendantsOf introduces a new Data.Map.Strict import solely for childrenByParent. The file already imports Data.HashMap.Strict qualified as HM and uses it everywhere; Text is Hashable. Replace:

-- LogQueries.hs
import Data.Map.Strict qualified as Map   -- ← remove this new import

childrenByParent :: Map.Map Text [V.Vector AE.Value]
childrenByParent = Map.fromListWith (<>) [(p, [r]) | r <- rows, Just p <- [pid' r]]

with:

childrenByParent :: HM.HashMap Text [V.Vector AE.Value]
childrenByParent = HM.fromListWith (<>) [(p, [r]) | r <- rows, Just p <- [pid' r]]

Same for Map.lookup s childrenByParent → HM.lookup s childrenByParent. This removes the new import entirely.

2. Latent bug: maybe True passes rows with no span-id unconditionally

newKids = filter (\r -> maybe True (`S.notMember` visited) (sid r)) kids

When sid r = Nothing (no latency_breakdown / span-id), the row always passes the filter and gets added to the result unconditionally on every BFS step it's reachable from. Change True → False so unidentifiable rows are dropped rather than spuriously included:

newKids = filter (\r -> maybe False (`S.notMember` visited) (sid r)) kids

3. if … then Just … else Nothing — use guard idiom

-- Commands.hs
, if opts.withChildren then Just ("with_children", "true") else Nothing

The existing list uses fmap ("key",) opt for Maybe values. For a Bool switch the idiomatic form (consistent with Relude) is:

, ("with_children", "true") <$ guard opts.withChildren

4. BFS accumulates result via acc <> newKids (list append)

in bfs visited' (rest <> newSids) (acc <> newKids)

acc <> newKids appends to the front accumulator each step, making the total O(n²) in the number of matched nodes. With the 2000-row cap this is negligible, but the standard fix is to accumulate in reverse and reverse once at the end:

bfs visited [] acc = (visited, reverse acc)
bfs visited (s : rest) acc =
  ...
  in bfs visited' (rest <> newSids) (newKids <> acc)  -- prepend instead

Minor

• The parameter order change in selectChildSpansAndLogs (inserting seedSpanIds before dateRange) is a breaking internal API change — worth a note in the PR description for any other callers.
• r.count shouldBe 1 in the new LogSpec test implicitly relies on the predicate matching exactly one span by unique marker, which is correct given nextRandom, but the assertion could be accompanied by a brief comment explaining it's the predicate hit count, not the logsData row count.

Summary

The core logic is correct and the test is a real regression guard. The main actionable items are:

1. Remove the new Map.Strict import — use the existing HM.HashMap (consistency + avoids an extra dependency surface).
2. Fix the maybe True → maybe False bug to avoid accidentally leaking span-less rows.
3. Prefer <$ guard for the CLI flag.

Code review posted below (body split to avoid shell quoting issues)

Code Review - issues found in keepDescendantsOf (LogQueries.hs): 1) Drop Data.Map.Strict import - HM.HashMap already imported gives O(1) avg vs O(log n). 2) O(n^2) list appends in BFS - rest <> newSids and acc <> newKids are O(n) each step; concat/reverse pattern fixes it. 3) maybe True should be maybe False in newKids filter - rows without span_id pass through but never enter visited so they cannot be de-duplicated. 4) V.toList seedSpanIds computed twice in bfs call. Positives: design rationale comment is exemplary, default exact-match behaviour is correct, sanity-check in tests is good, apiLogExpandH update is minimal.

Code Review

Overview: Fixes the regression where search returned every span sharing a trace_id with a predicate hit (siblings, parents, uncles). Now the API/CLI default is exact-match only; with_children=true opts into descendant expansion via an in-memory BFS over a bounded (≤2000 row) SQL scan. The approach and tests are solid — a few things worth tightening.

Bug / Correctness

bfs is DFS, not BFS (LogQueries.hs)

bfs visited' (newSids <> rest) (newKids <> acc)
--           ^^^^^^^^^^^^^^^^ prepend = stack = DFS

newSids <> rest pushes new nodes to the front of the worklist (stack discipline = DFS). The Haddock says "BFS-expands" and the helper is named bfs — both are wrong. Correctness is unaffected (all descendants are still reached), but the mismatch will mislead the next person who reads it. Either rename the helper to go/walk and drop "BFS" from the Haddock, or swap to append (rest <> newSids) for real BFS.

Code Succinctness / Style

pid' collides with convention (keepDescendantsOf)

In this codebase pid means Project ID everywhere. Using pid' as the local name for the parent-span-id lookup will trip up readers. Rename to parentId or getParent:

-- current
pid' r = lookupVecTextByKey r colIdxMap "parent_id"
-- suggested
parentId r = lookupVecTextByKey r colIdxMap "parent_id"

seedSpanIds extracted twice in buildLogResult

seedSpanIds = V.mapMaybe (\v -> lookupVecTextByKey v colIdxMap "latency_breakdown") requestVecs

This expression is not duplicated in the diff itself, but it mirrors the same pattern used three lines above for allTraceIds. Fine as-is, just noting the lambda could be (flip (lookupVecTextByKey ?? colIdxMap) "latency_breakdown") if (??) is already in scope — though the explicit lambda is clearer here.

Over-commenting (project style: comment the why, not the what)

Several blocks explain implementation mechanics that are legible from the code:

• The multi-line "Prepend kids and seeds to flip the standard append-build…" comment in keepDescendantsOf — the O(n) reverse trick is standard; one short line or no comment is enough.
• -- maybe False: rows missing a span_id (latency_breakdown) cannot be the root… — the code already says maybe False clearly; the sentence repeats it in prose.
• -- latency_breakdown is aliased from context___span_id (see Pkg.Parser). appears in two places; one is fine, two is noise.

Test Coverage

Near-identical fixture setup duplicated across two test files

LogSpec.hs and ApiV1Spec.hs both construct the same 5-span topology (root A → child B → grandchild C, unrelated root E → child D, same trace). The setup code is nearly identical. Extracting a shared helper in Pkg.TestUtils (e.g. mkDescendantFixture) would cut ~30 lines and make both tests easier to maintain.

Sanity-check assertion in LogSpec.hs is good — querying the DB to confirm the ingested shape before asserting on query results is the right call; keep it.

Minor

• selectChildSpansAndLogs signature — parameter order now is traceIds → seedSpanIds → dateRange → excludedSpanIds. seedSpanIds and excludedSpanIds both being V.Vector Text sitting around dateRange is easy to transpose; a small record or newtype would prevent silent mis-ordering, but the function is internal so not urgent.
• queryEvents positional Maybe chain — now 8 positional Maybe args. Pre-existing, but the addition of withChildrenM pushes it further. Worth a record type (EventsQueryParams) at some point.
• The (Nothing, Just b) branch in selectChildSpansAndLogs omits the ±30 s buffer that (Just a, Just b) applies — pre-existing, not introduced here, but now that seedSpanIds tightens the filter it's more visible.

Overall: the fix is correct, the in-memory descendant walk is well-reasoned (the "why not recursive CTE" Haddock is worth keeping), and the regression tests cover the exact failure mode. Address the bfs/DFS naming issue and the pid' confusion before merging; the rest is polish.

Code Review — Fix/events search descendants only

Overview

This PR fixes a significant correctness bug: the events search API was returning every span in a matching trace (siblings, uncles, unrelated roots), not just predicate hits and their descendants. The fix adds an in-memory BFS/DFS filter after fetching candidate spans via a single broad SQL scan, plus a new with_children query parameter and --with-children CLI flag to let callers opt in to tree-context expansion.

The approach is sound and well-tested. A few points worth addressing:

Bug: bfs does DFS, not BFS

bfs visited (s : rest) acc =
  ...
  in bfs visited' (newSids <> rest) (newKids <> acc)

newSids <> rest prepends new nodes to the front of the queue — that is a stack (DFS), not a queue (BFS). The function name, the docstring phrase "BFS-expands from the seed set", and the inline comment "Prepend kids and seeds to flip the standard 'append-build, then reverse' pattern" are all in conflict. For correctness (finding all descendants) it makes no difference, but the naming will mislead the next reader. Either rename to go/dfs or use rest <> newSids (true BFS).

fromMaybe [] (HM.lookup ...) → HM.findWithDefault []

-- new code
let kids = fromMaybe [] (HM.lookup s childrenByParent)

-- already used in this file (line 541, 544)
fromMaybe [] (HM.lookup h memberHashMap)

HM.findWithDefault [] s childrenByParent is shorter and the library's intended API for this pattern. Both are in-use, but the new code adds another instance of the longer form.

Overlong inline comments

keepDescendantsOf has two multi-line comment blocks that describe what the code does rather than a non-obvious why:

-- maybe False: rows missing a span_id (latency_breakdown) cannot be
-- the root of any further descent, and we have no key to dedup on,
-- so drop them rather than risk re-adding via another path.
--
-- Prepend kids and seeds to flip the standard "append-build, then
-- reverse" pattern into O(n) total instead of O(n²) from repeated
-- `acc <> newKids` / `rest <> newSids`.

The first block explains maybe False — readable from the expression itself. The second block explains the reverse-accumulator idiom — standard Haskell folklore. Both can be dropped; the docstring already covers the O(n) claim at the right level.

if V.null seedSpanIds … else do — nesting

if V.null seedSpanIds
  then pure []
  else do
    results <- …
    pure $ keepDescendantsOf …

The guard is already checked by keepDescendantsOf (| V.null seedSpanIds || null rows = []), so this adds a defensive double-check. If the intent is to skip the SQL call entirely, the nesting is justified — but since keepDescendantsOf gates on the same condition you could alternatively just always run the SQL and let the in-memory filter return []. If the early-return is worth keeping (it avoids a DB round-trip), a comment explaining that is helpful.

Seeds excluded from keepDescendantsOf output — undocumented assumption

The seeds are pre-loaded into visited and the accumulator starts empty, so seed spans are never added to acc — only strict descendants are returned. This is correct because the seeds already live in requestVecs. However keepDescendantsOf's type and docstring don't mention this: a caller that passes seeds not already in the result set would silently lose them. Worth one sentence in the Haddock.

Minor

• valueToVector could use preview AE._Array (lens-aeson) if _Array is already imported elsewhere; current case-expression is fine but marginally longer.
• HM.fromListWith (<>) in the new code vs HM.fromListWith (++) in the surrounding older code — both compile, prefer (<>) for consistency (new code already does this correctly).

Test coverage

The two new integration tests ("returns only matched spans plus their descendants…" and "with_children: default returns only predicate hits; flag adds descendants") are well-constructed and directly exercise the regression. The pagination cursor test is a good addition. The sanity-check ingestion query in the tree-expansion test is particularly useful for diagnosing future failures.

One note: pageIdsFor issues a second DB query per page to resolve attributes. This is fine for an integration test but adds latency and a dependency on context___span_id being stable — flagging only so reviewers know it's intentional.

Summary

The fix is correct, the SQL rationale is sound, and the test suite is solid. Main asks: rename bfs→go/dfs (or switch to true BFS order), prefer HM.findWithDefault over fromMaybe [] . HM.lookup, trim the inline comments to just the non-obvious invariants, and add a note to keepDescendantsOf's Haddock that seeds are not included in the output.

Code Review

Overview: Fixes the search-returns-whole-trace bug by introducing a seed-based BFS filter (keepDescendantsOf) that trims the broad trace_id SQL scan to only actual descendants of matched spans. Adds a with_children API param and CLI flag so the UI can opt into tree expansion while the API/CLI default to predicate-only results. Integration tests cover both the fix and a multi-page cursor regression.

Correctness

The BFS logic is correct: builds a parent_id → [row] adjacency, marks seeds as visited upfront, expands children level by level. The early-exit guard if V.null seedSpanIds then pure [] in selectChildSpansAndLogs is a good addition. The duplicate guard inside keepDescendantsOf (| V.null seedSpanIds || null rows = []) makes it safe to call in isolation too.

One subtle consequence: seeds serves double duty as both the initial BFS queue and the initial visited set, so seed spans themselves never appear in the output acc. This is intentional (seeds are already in requestVecs) but could surprise a future reader — a short comment -- seeds are already in requestVecs; mark visited to avoid re-adding would help.

Succinctness / packages we already have

HM.findWithDefault is available — fromMaybe [] (HM.lookup s childrenByParent) can be:

HM.findWithDefault [] s childrenByParent

Used elsewhere in the codebase (Widget.hs uses M.findWithDefault). One fewer import/wrapper.

Data.HashSet for visited: the module already imports Data.Set qualified as S. Since S.member/S.insert are O(log n) and HashSet equivalents are O(1) amortised, with 2 000-row cap it doesn't matter — but if you're going to hardcode the choice, HashSet is more idiomatic for a "visited" set. Low priority.

listToIndexHashMap in Utils.hs (pre-existing, exposed by this PR):

listToIndexHashMap list = HM.fromList [(x, i) | (x, i) <- zip list [0 ..]]

The list comprehension destructures (x, i) immediately just to rebuild (x, i) — equivalent to:

listToIndexHashMap = HM.fromList . flip zip [0..]

Worth fixing in Utils while you're touching this module.

Comments / verbosity

The inline block inside keepDescendantsOf is over-commented relative to project style. The -- maybe False: rows missing a span_id... paragraph and the -- Prepend kids and seeds to flip the standard "append-build..." block explain straightforward Haskell accumulator pattern. Trim to a single short note on the non-obvious invariant (seeds already in visited). The Haddock block on selectChildSpansAndLogs is good — keep it.

API surface

The Bool positional arg added to buildLogResult is the main style flag — at call sites buildLogResult True … and buildLogResult (fromMaybe False withChildrenM) … are readable enough given this codebase's conventions. Fine as-is.

The colNames local binding in selectChildSpansAndLogs contains raw SQL expressions (expr as alias), while colNames elsewhere (e.g. returned from selectLogTable) is already processed by listToColNames. The existing comment explains this, but renaming the local binding to rawColExprs would remove the ambiguity without any other changes.

Tests

Solid. The ingest-then-sanity-check pattern in the tree expansion test (asserting the persisted row names before running the search) is good defensive testing. The cursor pagination test is well-structured with clear per-page expectations. Both tests make good use of real DB rather than mocks, consistent with project convention.

Minor: pageIdsFor does a per-span SQL round-trip to retrieve page.idx; fine for a test but the function could be documented with a brief note that it issues N queries.

Summary

No correctness bugs found. Actionable improvements in order of priority:

1. HM.findWithDefault [] s childrenByParent (drop fromMaybe [])
2. Fix listToIndexHashMap in Utils to HM.fromList . flip zip [0..]
3. Rename colNames → rawColExprs in selectChildSpansAndLogs
4. Add one-line comment on why seeds initialises visited in keepDescendantsOf
5. Trim the two verbose inline comment blocks inside keepDescendantsOf

Code Review — Fix/events search descendants only

Summary

Fixes a long-standing correctness bug: the events search was returning every span in a matched trace (siblings, parents, uncles) rather than just predicate hits. The fix adds a withChildren flag (defaults to False) and a post-SQL in-memory tree walk (keepDescendantsOf) that prunes the broad fetch to only strict descendants of matched spans.

Correctness

keepDescendantsOf — traversal order is neither BFS nor clean DFS

go visited' (newSids <> rest) (newKids <> acc)
-- ^^ prepend children = DFS order for the work queue
-- but newKids <> acc + reverse acc at the end produces an unpredictable output order

newSids <> rest makes the queue DFS, but newKids <> acc combined with the final reverse produces an ordering that is neither depth-first nor breadth-first. For a tree A → [B, C], B → [D], the output is [C, B, D]. If callers need a predictable order, this should be explicitly stated or fixed. A plain BFS (rest <> newSids) with acc <> newKids (or a DList) would be cleaner.

HM.fromListWith (<>) reverses child order within each parent

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

fromListWith f [(k, v1), (k, v2)] = f v2 v1 — so (<>) here appends later-found rows before earlier ones, reversing the iteration order of rows. Use HM.fromListWith (flip (<>)) to preserve the original rows ordering within each children list.

Silent failure risk — hardcoded column aliases

keepDescendantsOf and its two call sites hardcode "latency_breakdown" and "parent_id" as plain Text lookups:

-- LogQueries.hs
sid r = lookupVecTextByKey r colIdxMap "latency_breakdown"
pid' r = lookupVecTextByKey r colIdxMap "parent_id"

-- Log.hs (two call sites)
seedSpanIds = V.mapMaybe (\v -> lookupVecTextByKey v colIdxMap "latency_breakdown") requestVecs

If the context___span_id as latency_breakdown alias ever changes in Parser.hs, all three sites silently return Nothing/empty — keepDescendantsOf returns [], seedSpanIds becomes empty, and selectChildSpansAndLogs short-circuits to pure []. No type error, no warning. Consider defining a small constant or pulling it from the same source as the default select column list.

API/type nits

withChildren :: Maybe Bool vs Bool

The Maybe Bool in EventsQuery only exists to distinguish "caller didn't pass it" from False, but fromMaybe False collapses them immediately in queryEvents. A Bool with a Default instance or deriving Default on the field would be simpler. The Maybe adds noise at every usage site.

Signature growth in buildLogResult

buildLogResult :: (DB es, Time.Time :> es) => Bool -> Projects.ProjectId -> UTCTime -> Maybe Text -> Maybe Text -> Maybe Text -> [Text] -> (...) -> Eff es LogResult

The function now has 9 positional arguments, the first four of which are the same type (Bool, ProjectId, UTCTime, Maybe Text). Easy to swap silently. A small options record (BuildLogOpts) or at minimum a named field for withChildren would help. Not blocking, but worth noting given this function is called from multiple places.

Performance

The strategy comment in the haddock is good. One additional note: the 2000-row LIMIT in the SQL is a hard cap on the fetch, but keepDescendantsOf then allocates HM.fromListWith over all 2000 rows regardless of how many actually match. For the common case where there are only a handful of seed spans, this is dominated by the query RTT and is fine.

Tests

The regression tests are well-structured:

• The shouldBe ["ui.tree.child", ...] sanity check before asserting row counts is a nice pattern — it catches "the ingestion topology didn't persist as expected" separately from "the filter logic is wrong."
• The three-page cursor test with shouldSatisfy (/= page1Cursor) regression hedge is a good addition.
• Coverage hits the key boundary: sibling spans that share a trace_id but are not descendants of the predicate match are excluded. ✓

Minor gap: there's no explicit test for the V.null seedSpanIds → pure [] early-exit path in selectChildSpansAndLogs. A single test that sends a query matching zero spans with with_children=true would close it.

Misc style

• The colNames variable in selectChildSpansAndLogs is used only to build listToIndexHashMap (listToColNames colNames) inside the else branch. You can bind it lazily there rather than at the top of the let.
• colNames from getProcessedColumns already has as-aliased entries; the subsequent listToColNames call is correct but the intent ("strip to bare aliases") deserves a one-liner binding name like bareColNames instead of an inline comment.