[karl-power]

Summary

On ClickHouse Cloud, read MergeTree settings from system.shared_merge_tree_settings, falling back to system.merge_tree_settings for each setting, when detecting persisted _block_number and _block_offset columns.

• Metadata.getMergeTreeSettings also reads system.shared_merge_tree_settings on Cloud (detected with isClickHouseCloud). Where both tables set a key, the shared value wins
• Outside Cloud it still reads only system.merge_tree_settings, since OSS servers don't have the shared table
• If the shared read fails on Cloud, it returns undefined (not cached), so the next call retries
• Unit tests for the non-Cloud path, the Cloud per-key preference, and the Cloud failure case

The code is identical to the EE change, so the forward-port should merge without conflicts.

How to reproduce

Starting point: a ClickHouse Cloud service where block columns are enabled in the shared settings but not in system.merge_tree_settings. Check with:

SELECT 'merge_tree' AS t, name, value FROM system.merge_tree_settings WHERE name LIKE 'enable_block_%'
UNION ALL
SELECT 'shared', name, value FROM system.shared_merge_tree_settings WHERE name LIKE 'enable_block_%'

Add a log table on that service as a source, with no block-column settings in its SETTINGS clause, and ingest some rows.

On main:

1. Open the source in search and click a row.
2. In the ClickHouse query log, the row lookup doesn't use _block_number / _block_offset and falls back to matching the full row.

On this branch:

1. Repeat the steps above.
2. The row lookup now selects and filters on _block_number and _block_offset.

How to test on Vercel preview

N/A — non-UI change.

References

• Linear Issue: O11Y-29693
• Related PRs: backport of https://github.com/DeploySentinel/hyperdx-ee/pull/3925, follows feat(app): fall back to server MergeTree settings for block columns #3530

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 6, 2026 8:40am UTC
hyperdx-storybook	Ready	Preview	Nov 6, 2026 8:40am UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: 22dcc5e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/common-utils	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

• Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats

• Production files changed: 1
• Production lines changed: 40 (+ 83 in test files, excluded from tier calculation)
• Branch: claude/backport-shared-merge-tree-settings
• Author: karl-power

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

🟠 major — A failed Cloud check permanently caches settings that ignore the shared table

If the Cloud check fails once (a timeout or network blip), isClickHouseCloud (metadata.ts:2146-2173) doesn't cache the failure but returns false. getMergeTreeSettings then builds a map from merge_tree_settings alone. MetadataCache.set stores it with no TTL (metadata.ts:304-330), so a Cloud connection keeps missing the shared-table values for the life of the session. That is exactly the bug this PR is meant to fix. Fix: if detection fails, don't cache this result either. For example, add a method that returns boolean | undefined, and return undefined from getMergeTreeSettings when the result is undefined.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

🟠 major — A failed shared-table read throws away the merge_tree_settings result too

Both queries run in one Promise.all inside one try, so any error reading system.shared_merge_tree_settings makes the whole call return undefined. That includes 'Not enough privileges' for a restricted Cloud user, a case getSettings (metadata.ts:2226-2236) already handles. Block-column detection then fails, although on main it worked from merge_tree_settings. And since undefined isn't cached, every call re-runs both queries (useMergeTreeSettings uses staleTime 0 on failure). This contradicts the 'falls back to merge_tree_settings per key' claim in the changeset. Fix: catch the shared read on its own (e.g. .catch(() => [])) so the merge_tree_settings rows are still used.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

3 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 1 minor

2 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/common-utils/src/__tests__/metadata.test.ts:100 — Tests lock in the discard-on-failure behaviour and never cover a failed Cloud check → 'returns undefined when the shared read fails on Cloud' asserts the regression described above. No test covers isClickHouseCloud failing. That case is hidden because every test stubs it with jest.spyOn(...).mockResolvedValue. Fix: change the test to expect the merge_tree_settings values when the shared read fails. Add a test that uses a real MetadataCache and makes the system.table_engines query reject, then asserts that a second call re-queries instead of returning a cached map without the shared values.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.22%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

Deep Review

feat(app): prefer shared MergeTree settings — on ClickHouse Cloud, Metadata.getMergeTreeSettings now also reads system.shared_merge_tree_settings (gated by isClickHouseCloud) and merges it over system.merge_tree_settings per key. Scope is small and well-tested; the merge-override ordering is correct and asserted. One reliability edge on the Cloud failure path is worth a look; everything else is nits.

🟡 P2 — recommended

• packages/common-utils/src/core/metadata.ts:2271 — On Cloud, Promise.all reject-fast means a failing system.shared_merge_tree_settings read discards the already-successful merge_tree_settings rows and returns undefined, so a user who cannot read the shared table (or hits a persistent error) now gets no server settings at all, where before this change they received the merge_tree_settings map — degrading block-column detection to a full-row lookup for that connection.
  • Fix: Wrap only the shared read (e.g. Promise.allSettled or a try/catch around the shared readSettings call) so its failure falls back to the merge_tree_settings rows instead of discarding them.
  • _{correctness, reliability, kieran-typescript, testing}

🔵 P3 nitpicks (4)

• packages/common-utils/src/core/metadata.ts:2260 — The readSettings helper omits shouldSkipApplySettings: true, which the sibling system-catalog readers (isClickHouseCloud, getServerVersion, getSettings) all pass; this matches the method's own pre-existing merge_tree_settings read, but the new shared query inherits it too and routes a system-table read through optimization/user settings.
  • Fix: Pass shouldSkipApplySettings: true on both reads in readSettings for consistency with the other system-catalog readers.
• packages/common-utils/src/core/metadata.ts:2270 — A transient failure of the isClickHouseCloud detection query yields isCloud=false for that call and caches a merge_tree_settings-only map (map results are cached, unlike undefined), so the Cloud shared settings stay unapplied for the connection's lifetime even once detection would succeed.
  • Fix: Distinguish "detected non-Cloud" from "detection failed" and avoid caching the merge-tree-only result when Cloud status could not be positively determined.
• packages/common-utils/src/__tests__/metadata.test.ts:599 — No Cloud test covers the shared read succeeding with zero rows, leaving the "empty shared table does not wipe out merge_tree values" edge unverified (distinct from the override case).
  • Fix: Add a Cloud case with isClickHouseCloud=true and sharedMergeTree: [] asserting the result equals the merge_tree_settings map.
• packages/common-utils/src/__tests__/metadata.test.ts:594 — Full-literal query-string assertions ('SELECT name, value FROM system.merge_tree_settings') couple the tests to exact SQL formatting, so a whitespace or column-order refactor breaks them without a behavior change.
  • Fix: Assert with expect.stringContaining('system.merge_tree_settings') / 'system.shared_merge_tree_settings' to verify the correct table without binding to exact text.

---

Reviewers (7): correctness, kieran-typescript, testing, reliability, maintainability, performance, project-standards.

Testing gaps: Cloud path with an empty (zero-row) shared_merge_tree_settings result is uncovered. The merge-override ordering and the shared-read-failure→undefined path are both already asserted; the changeset package list and bump types match repo conventions.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

E2E Test Results

✅ All tests passed • 464 passed • 1 skipped • 1446s

Status	Count
✅ Passed	464
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[jordan-simonovski]

Looks good!