Repository navigation
feat(app): fall back to server MergeTree settings for block columns - #3530
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 4696fae The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const json = await this.clickhouseClient | ||
| .query<'JSON'>({ | ||
| connectionId, | ||
| query: 'SELECT name, value FROM system.merge_tree_settings', |
There was a problem hiding this comment.
🔵 minor — Fallback reads only system.merge_tree_settings, but ReplicatedMergeTree tables take <replicated_merge_tree> overrides first
The engine check at DBRowTable.tsx:1606 (engine.includes('MergeTree')) lets in ReplicatedMergeTree tables. Distributed tables also land here, because getTableMetadata (metadata.ts:1997-2009) swaps in the local table's engine, which is usually ReplicatedMergeTree. For those tables, ClickHouse applies <replicated_merge_tree> config (visible in system.replicated_merge_tree_settings) on top of <merge_tree>. Reading only merge_tree_settings can therefore return the wrong effective value. One case: merge_tree enables a block column and replicated_merge_tree turns it off. The row lookup then keys on _block_number/_block_offset even though they aren't persisted, so it can fetch the wrong row. Fix: for Replicated* engines, also query system.replicated_merge_tree_settings and let its values override the merge_tree_settings ones before falling back.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor 1 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. The change is well-scoped and mirrors existing cached-metadata patterns; the cache-retry-on-failure semantics ( 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (3 returned): maintainability, performance, testing. Correctness, project-standards, reliability, kieran-typescript, and adversarial were dispatched but had not returned at synthesis time; the P2 server-default finding comes from the orchestrator's own code-grounded analysis of ClickHouse MergeTree semantics. Testing gaps: The "avoid querying twice" guarantee is only verified indirectly (config returns 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 |
E2E Test Results✅ All tests passed • 458 passed • 1 skipped • 1867s
Tests ran across 4 shards in parallel. |
A single persisted block column combined with the primary key does not identify a row: _block_offset repeats across insert blocks, _block_number across the rows of one block, and exporter-default tables key on a second-precision TimestampTime. Since #3530 the block columns are also detected from server-level merge_tree_settings, so any table without explicit settings took the key-only path on servers that enable only one of them and could open the wrong row. Use the key-only lookup only when both block columns are persisted; a lone block column is still appended to the full-row lookup as a tiebreaker.
…kup (#3566) ## Summary Clicking a search result could open the wrong row's details when the table persists only one of `_block_number` / `_block_offset`. - Since #3364 the side panel used a key-only lookup (primary key + partition + whichever block column exists) when at least one block column was persisted. One block column is not unique: `_block_offset` repeats across insert blocks, `_block_number` across the rows of one block, and the primary key often doesn't break the tie (the OTel exporter default keys on the second-precision `TimestampTime`). - Since #3530 the block columns are also detected from server-level `merge_tree_settings`, so on servers that enable only one of them, every table without explicit settings takes this path. - The key-only lookup now requires both block columns. A lone block column is still appended to the full-row lookup as a tiebreaker. - Tables that persist only one block column return to the full-row lookup and lose the #3364 speed-up. A `_part` + `_part_offset` lookup is the follow-up to restore it. ### How to reproduce **Starting point:** `yarn dev`. Set the server default on the dev ClickHouse and restart it: ```sh docker exec <ch-server> sh -c 'echo "<clickhouse><merge_tree><enable_block_offset_column>1</enable_block_offset_column></merge_tree></clickhouse>" > /etc/clickhouse-server/config.d/block_offset.xml' docker restart <ch-server> ``` Create an exporter-shaped table and insert three rows as separate inserts, two in the same second: ```sql CREATE TABLE default.repro_logs ( Timestamp DateTime64(9), TimestampTime DateTime DEFAULT toDateTime(Timestamp), ServiceName LowCardinality(String), SeverityText LowCardinality(String), Body String, LogAttributes Map(LowCardinality(String), String) ) ENGINE = MergeTree PARTITION BY toDate(TimestampTime) PRIMARY KEY (ServiceName, TimestampTime) ORDER BY (ServiceName, TimestampTime, Timestamp); INSERT INTO default.repro_logs (Timestamp, ServiceName, SeverityText, Body) VALUES ('2026-11-05 09:00:39.845', 'saruman', 'INFO', 'Response x_span_id=ESLMARS7L7'); INSERT INTO default.repro_logs (Timestamp, ServiceName, SeverityText, Body) VALUES ('2026-11-05 09:00:39.100', 'saruman', 'INFO', 'Request x_span_id=ESOPOZ7FCV'); INSERT INTO default.repro_logs (Timestamp, ServiceName, SeverityText, Body) VALUES ('2026-11-05 09:00:36.909', 'saruman', 'INFO', 'Request x_span_id=ESLMARS7L7'); ``` Add a log source for `default.repro_logs` (timestamp column `TimestampTime`), then search it for `"ESLMARS7L7"` over a range covering 2026-11-05 09:00 UTC. **On `main`:** 1. Click the `Response x_span_id=ESLMARS7L7` row a few times. 2. The side panel shows `Request x_span_id=ESOPOZ7FCV` about half the time. The query in `system.query_log` is `WHERE ServiceName='saruman' AND TimestampTime=... AND toDate(TimestampTime)=... AND _block_offset='0' LIMIT 1`. **On this branch:** 1. Click the same row. The panel always shows `Response x_span_id=ESLMARS7L7`; the query also filters on `Timestamp`, `SeverityText` and `Body`. ### How to test on Vercel preview N/A — needs a ClickHouse server default; the demo server's user cannot read `system.merge_tree_settings`. ### References - Related PRs: #3530, #3364, #2320, #2071
## 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: ```sql 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 DeploySentinel/hyperdx-ee#3925, follows #3530
Summary
Detect persisted
_block_numberand_block_offsetcolumns from the server's MergeTree settings when the table's CREATE query doesn't set them, so row lookups can skip the slow full-row match on those tables.Metadata.getMergeTreeSettings, which readssystem.merge_tree_settingsand caches it per connection (returnsundefinedon failure so the next call retries)useMergeTreeSettingshook around itgetPersistedBlockColumnsnow parses the table'sSETTINGSclause and falls back to the server value for any block-column setting the table omits; table-level settings still winHow to reproduce
Starting point: local ClickHouse with block columns turned on server-wide, e.g. in
config.xml:Create a log table without those settings in its
SETTINGSclause, add it as a source, and ingest some rows.On
main:_block_number/_block_offsetand falls back to matching the full row.On this branch:
_block_numberand_block_offset.References