From 335c8f96c04922a91764d5b511d9fb4ac20f2be0 Mon Sep 17 00:00:00 2001
From: Warren Lee <5959690+wrn14897@users.noreply.github.com>
Date: Mon, 17 Aug 2026 22:11:21 -0700
Subject: [PATCH 1/3] fix(common-utils,api): restore filter sidebar values
 behind query proxies

Batched facet-value queries (KV rollup / map text-index) bound one query
parameter per key. With ~100 sidebar keys this exceeds the ClickHouse web
client's URL parameter budget (MAX_URL_BIND_PARAM_LENGTH = 4096), which
silently promotes the request to a multipart/form-data body. Managed query
gateways reject that body with HTTP 400, the strategy swallows the error,
and every LowCardinality-column and map-attribute filter disappears from
the sidebar with no visible failure (ClickHouse support-escalation #8482).

- Inline facet keys as SQL-escaped literals in getMetadataMVKeyValues and
  getMapTextIndexKeyValues so the payload rides the POST body and the
  parameter count stays constant regardless of key count.
- Parenthesize the KV rollup OR chain: 'a OR b AND timeFilter' previously
  bound the time filter (and notEmpty) to the last branch only.
- Harden /clickhouse-proxy body forwarding: pass unparsed bodies (e.g.
  multipart/form-data) through untouched, match application/json with a
  charset suffix, re-serialize urlencoded bodies, keep Content-Length in
  sync with the re-injected payload, and fail the proxied request loudly
  instead of forwarding a body-less query upstream.
- Integration coverage runs the real http-proxy-middleware: the int jest
  config transpiles the ESM-only proxy packages instead of blanket-mocking
  them.
---
 .changeset/fix-filter-facets-behind-proxy.md  |  16 ++
 packages/api/jest.int.config.js               |  23 ++
 packages/api/jest.setup.ts                    |   4 +-
 .../api/__tests__/clickhouseProxy.int.test.ts | 201 ++++++++++++++++++
 .../api/src/routers/api/clickhouseProxy.ts    |  73 ++++++-
 .../src/__tests__/metadata.test.ts            | 119 ++++++++++-
 packages/common-utils/src/core/metadata.ts    |  35 ++-
 7 files changed, 447 insertions(+), 24 deletions(-)
 create mode 100644 .changeset/fix-filter-facets-behind-proxy.md
 create mode 100644 packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts

diff --git a/.changeset/fix-filter-facets-behind-proxy.md b/.changeset/fix-filter-facets-behind-proxy.md
new file mode 100644
index 0000000000..2865e8d528
--- /dev/null
+++ b/.changeset/fix-filter-facets-behind-proxy.md
@@ -0,0 +1,16 @@
+---
+"@hyperdx/common-utils": patch
+"@hyperdx/api": patch
+---
+
+Fix filter sidebar values disappearing behind query proxies. Batched facet-value
+queries (KV rollup and map text-index lookups) previously bound one query
+parameter per key; with ~100 keys this exceeded the ClickHouse web client's URL
+parameter budget, silently promoting the request to a multipart/form-data body
+that proxy gateways can reject — every LowCardinality-column and map-attribute
+filter then vanished without an error. Keys are now inlined as SQL-escaped
+literals so the query rides the POST body. Also fixes an operator-precedence
+bug that applied the rollup time filter to only the last OR branch, and hardens
+the API's /clickhouse-proxy body forwarding (multipart passthrough, charset-
+suffixed JSON content types, urlencoded re-serialization, accurate
+Content-Length).
diff --git a/packages/api/jest.int.config.js b/packages/api/jest.int.config.js
index add0482623..eb316f8991 100644
--- a/packages/api/jest.int.config.js
+++ b/packages/api/jest.int.config.js
@@ -1,8 +1,31 @@
+const { createJsWithTsPreset } = require('ts-jest');
+
 const base = require('./jest.config.js');
 
+// http-proxy-middleware v4 (and its proxy core, httpxy) ship ESM-only builds
+// that Jest's CJS loader cannot parse. The unit config blanket-mocks the
+// package (see jest.setup.ts), but the clickhouse-proxy integration tests
+// exercise the real proxy, so the int config transpiles those two packages to
+// CJS via ts-jest instead. `allowJs` lets ts-jest compile their plain-JS ESM
+// sources; the extra `.mjs` transform covers httpxy's `dist/index.mjs` entry.
+const esmDepsTransformCfg = createJsWithTsPreset({
+  tsconfig: {
+    rootDir: './src',
+    allowJs: true,
+  },
+});
+
 /** @type {import("jest").Config} **/
 module.exports = {
   ...base,
+  transform: {
+    ...esmDepsTransformCfg.transform,
+    '^.+\\.mjs$': esmDepsTransformCfg.transform['^.+\\.[tj]sx?$'],
+  },
+  moduleFileExtensions: ['js', 'mjs', 'json', 'ts', 'tsx', 'node'],
+  transformIgnorePatterns: [
+    '/node_modules/(?!(http-proxy-middleware|httpxy|is-plain-obj)/)',
+  ],
   testMatch: ['**/__tests__/**/*.int.test.ts?(x)'],
   // Override the unit config's ignore list: it excludes `.int.test.ts`, which
   // would otherwise hide every integration test from this suite.
diff --git a/packages/api/jest.setup.ts b/packages/api/jest.setup.ts
index 272998b094..d7cefbb50f 100644
--- a/packages/api/jest.setup.ts
+++ b/packages/api/jest.setup.ts
@@ -11,7 +11,9 @@ jest.retryTimes(
 );
 
 // http-proxy-middleware v4 is ESM-only and Jest's CJS module loader cannot
-// load ESM packages. Auto-mock since no test exercises the proxy directly.
+// load ESM packages. Auto-mock it for suites that don't exercise the proxy.
+// The clickhouse-proxy integration tests need the real thing: they opt out
+// via jest.unmock(), and jest.int.config.js transpiles the package to CJS.
 jest.mock('http-proxy-middleware', () => ({
   createProxyMiddleware: jest.fn(() => jest.fn()),
 }));
diff --git a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
new file mode 100644
index 0000000000..60e719bb55
--- /dev/null
+++ b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
@@ -0,0 +1,201 @@
+import http from 'http';
+
+import { getLoggedInAgent, getServer } from '@/fixtures';
+import Connection from '@/models/connection';
+
+// jest.setup.ts blanket-mocks http-proxy-middleware (ESM-only) for suites that
+// don't touch it; this suite exercises the real proxy, which the int jest
+// config makes loadable by transpiling it (see jest.int.config.js).
+jest.unmock('http-proxy-middleware');
+
+type CapturedRequest = {
+  method: string | undefined;
+  url: string | undefined;
+  headers: http.IncomingHttpHeaders;
+  body: Buffer;
+};
+
+/**
+ * Verifies the /clickhouse-proxy route forwards request bodies to the upstream
+ * ClickHouse host byte-for-byte, across every content type the web client
+ * emits.
+ *
+ * Regression context (ClickHouse support-escalation #8482): when
+ * `@clickhouse/client-web` promotes oversized query params to a
+ * multipart/form-data body (`use_multipart_params_auto`), no express body
+ * parser consumes the stream and the proxy used to attempt
+ * `proxyReq.write(req.body)` with the unparsed `{}` placeholder — throwing,
+ * swallowing the error, and (depending on the proxy internals) corrupting the
+ * upstream request. Filter sidebar facet queries then failed silently.
+ */
+describe('clickhouse-proxy body forwarding', () => {
+  const server = getServer();
+
+  let upstream: http.Server;
+  let upstreamUrl: string;
+  let captured: CapturedRequest[] = [];
+
+  beforeAll(async () => {
+    await server.start();
+
+    upstream = http.createServer((req, res) => {
+      const chunks: Buffer[] = [];
+      req.on('data', chunk => chunks.push(chunk));
+      req.on('end', () => {
+        captured.push({
+          method: req.method,
+          url: req.url,
+          headers: req.headers,
+          body: Buffer.concat(chunks),
+        });
+        res.writeHead(200, { 'Content-Type': 'application/json' });
+        res.end(JSON.stringify({ data: [] }));
+      });
+    });
+    await new Promise<void>(resolve => upstream.listen(0, resolve));
+    const address = upstream.address();
+    if (address == null || typeof address === 'string') {
+      throw new Error('Expected upstream server to listen on a TCP port');
+    }
+    upstreamUrl = `http://127.0.0.1:${address.port}`;
+  });
+
+  beforeEach(() => {
+    captured = [];
+  });
+
+  afterEach(async () => {
+    await server.clearDBs();
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((resolve, reject) =>
+      upstream.close(err => (err ? reject(err) : resolve())),
+    );
+    await server.stop();
+  });
+
+  const createConnection = async (teamId: string) => {
+    const connection = await Connection.create({
+      team: teamId,
+      name: 'Upstream Echo',
+      host: upstreamUrl,
+      username: 'default',
+      password: '',
+    });
+    return connection._id.toString();
+  };
+
+  it('forwards a text/plain SQL body verbatim', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    const sql = 'SELECT count() FROM system.tables FORMAT JSON';
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-text')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', 'text/plain')
+      .send(sql)
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    expect(captured[0].body.toString()).toBe(sql);
+    expect(captured[0].headers['content-length']).toBe(
+      String(Buffer.byteLength(sql)),
+    );
+    // Connection credentials are attached server-side.
+    expect(captured[0].headers['x-clickhouse-user']).toBe('default');
+  });
+
+  it('forwards a large multipart/form-data body byte-for-byte (unparsed stream passthrough)', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    // Mirror @clickhouse/client-web's use_multipart_params_auto output: a
+    // `query` part plus one `param_*` part per bound parameter, well past the
+    // 4096-byte URL budget that triggers the multipart promotion.
+    const boundary = '----clickhouse-js-test-boundary';
+    const parts: string[] = [
+      `--${boundary}\r\n` +
+        `Content-Disposition: form-data; name="query"\r\n\r\n` +
+        `SELECT Key FROM rollup WHERE Key IN ({p0:String}) FORMAT JSON\r\n`,
+    ];
+    for (let i = 0; i < 120; i++) {
+      parts.push(
+        `--${boundary}\r\n` +
+          `Content-Disposition: form-data; name="param_HYPERDX_PARAM_${i}"\r\n\r\n` +
+          `some.rather.long.map.attribute.key.number.${i}\r\n`,
+      );
+    }
+    parts.push(`--${boundary}--\r\n`);
+    const body = parts.join('');
+    expect(Buffer.byteLength(body)).toBeGreaterThan(4096);
+
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-multipart')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', `multipart/form-data; boundary=${boundary}`)
+      .send(body)
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    expect(captured[0].headers['content-type']).toBe(
+      `multipart/form-data; boundary=${boundary}`,
+    );
+    expect(captured[0].body.toString()).toBe(body);
+    expect(captured[0].headers['content-length']).toBe(
+      String(Buffer.byteLength(body)),
+    );
+  });
+
+  it('re-serializes an application/json body with a charset suffix', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    const payload = { query: 'SELECT 1', nested: { a: 1 } };
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-json')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', 'application/json; charset=utf-8')
+      .send(JSON.stringify(payload))
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    const forwarded = captured[0].body.toString();
+    expect(JSON.parse(forwarded)).toEqual(payload);
+    // Content-Length must match the re-serialized bytes actually sent, not
+    // the original request's header.
+    expect(captured[0].headers['content-length']).toBe(
+      String(Buffer.byteLength(forwarded)),
+    );
+  });
+
+  it('re-serializes an application/x-www-form-urlencoded body', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-urlencoded')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', 'application/x-www-form-urlencoded')
+      .send('query=SELECT+1&param_foo=bar')
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    const forwarded = new URLSearchParams(captured[0].body.toString());
+    expect(forwarded.get('query')).toBe('SELECT 1');
+    expect(forwarded.get('param_foo')).toBe('bar');
+  });
+
+  it('rejects requests without a connection id header', async () => {
+    const { agent } = await getLoggedInAgent(server);
+
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-no-conn')
+      .set('Content-Type', 'text/plain')
+      .send('SELECT 1')
+      .expect(400);
+
+    expect(captured).toHaveLength(0);
+  });
+});
diff --git a/packages/api/src/routers/api/clickhouseProxy.ts b/packages/api/src/routers/api/clickhouseProxy.ts
index e41946c1b0..f8247844b7 100644
--- a/packages/api/src/routers/api/clickhouseProxy.ts
+++ b/packages/api/src/routers/api/clickhouseProxy.ts
@@ -261,21 +261,71 @@ const proxyMiddleware: RequestHandler =
           return res.sendStatus(405);
         }
 
-        let body = _req.body;
-        if (_req.headers['content-type'] === 'application/json') {
+        // Re-inject the body only when an express body parser consumed the
+        // request stream (json / text / urlencoded). Any other content type —
+        // notably the multipart/form-data bodies @clickhouse/client-web emits
+        // when query params exceed its URL budget (use_multipart_params_auto)
+        // — leaves the stream unconsumed, and the proxy pipes it upstream
+        // verbatim; writing the unparsed `req.body` placeholder here would
+        // corrupt (or previously, error out of) that forwarding.
+        const contentType = _req.headers['content-type'] ?? '';
+        const body = _req.body;
+        let payload: string | Buffer | undefined;
+        if (typeof body === 'string' || Buffer.isBuffer(body)) {
+          // express.text() output (the common case for ClickHouse SQL bodies).
+          payload = body;
+        } else if (contentType.startsWith('application/json')) {
+          // express.json() parsed the stream — re-serialize. startsWith, not
+          // strict equality: clients commonly append `; charset=utf-8`.
           try {
-            body = JSON.stringify(body);
+            payload = JSON.stringify(body);
           } catch (e) {
-            console.error(e);
+            console.error('clickhouseProxy failed to serialize JSON body', e);
           }
+        } else if (
+          contentType.startsWith('application/x-www-form-urlencoded') &&
+          body != null &&
+          typeof body === 'object'
+        ) {
+          // express.urlencoded() parsed the stream — re-serialize.
+          const params = new URLSearchParams();
+          for (const [key, value] of Object.entries(body)) {
+            if (Array.isArray(value)) {
+              for (const item of value) params.append(key, String(item));
+            } else {
+              params.append(key, String(value));
+            }
+          }
+          payload = params.toString();
+        } else {
+          // Stream was not consumed by any parser — let it pipe through.
+          return;
+        }
+
+        if (payload == null) {
+          // The stream was consumed but we couldn't reconstruct the body.
+          // Abort loudly instead of forwarding a body-less query upstream,
+          // which surfaces as an opaque 400 from ClickHouse.
+          proxyReq.destroy(new Error('Failed to reconstruct request body'));
+          return;
         }
 
         try {
-          proxyReq.write(body);
-        } catch {
+          // Re-serialization can change the byte length relative to the
+          // original Content-Length header the proxy forwards; keep them in
+          // sync so the upstream doesn't read a truncated/over-long body.
+          if (!_req.headers['transfer-encoding']) {
+            proxyReq.setHeader('content-length', Buffer.byteLength(payload));
+          }
+          proxyReq.write(payload);
+        } catch (e) {
           console.error(
             `clickhouseProxy error writing body, body is type ${typeof body}`,
+            e,
           );
+          // Fail the proxied request rather than silently sending no body;
+          // the error handler below responds to the client.
+          proxyReq.destroy(e instanceof Error ? e : new Error(String(e)));
         }
       },
       proxyRes: (proxyRes, _req, res) => {
@@ -318,10 +368,13 @@ const proxyMiddleware: RequestHandler =
             typeof startedAt === 'number' ? performance.now() - startedAt : 0,
         });
         console.error('Proxy error:', err);
-        (_res as Response).writeHead(500, {
-          'Content-Type': 'application/json',
-        });
-        _res.end(
+        const response = _res as Response;
+        if (!response.headersSent) {
+          response.writeHead(500, {
+            'Content-Type': 'application/json',
+          });
+        }
+        response.end(
           JSON.stringify({
             success: false,
             error: err.message || 'Failed to connect to ClickHouse server',
diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts
index f238321e89..c3b356cede 100644
--- a/packages/common-utils/src/__tests__/metadata.test.ts
+++ b/packages/common-utils/src/__tests__/metadata.test.ts
@@ -951,7 +951,7 @@ describe('Metadata', () => {
         ]),
       );
 
-      jest
+      const doMVsAggregateSpy = jest
         .spyOn(metadata as any, 'doMetadataMVsAggregateColumn')
         .mockImplementation((...args: any[]) => {
           const columnName = args[1] as string;
@@ -959,6 +959,10 @@ describe('Metadata', () => {
             columnName === 'ServiceName' || columnName === 'SeverityText',
           );
         });
+
+      // Returned so individual tests can widen/replace the MV-aggregation
+      // answer without re-spying on the private method.
+      return { doMVsAggregateSpy };
     };
 
     afterEach(() => {
@@ -1020,8 +1024,106 @@ describe('Metadata', () => {
       expect(sql).toContain('BY ColumnIdentifier, Key');
       expect(sql).not.toContain('mergeTreeTextIndex(');
       expect(Object.values(params)).toContain('otel_logs_kv_rollup_15m');
-      expect(Object.values(params)).toContain('ServiceName');
-      expect(Object.values(params)).toContain('SeverityText');
+      // Keys are inlined as SQL-escaped literals (NOT bound as query
+      // parameters): one bind param per key blows past the web client's URL
+      // parameter budget for large filter batches, silently promoting the
+      // request to a multipart/form-data body that query gateways reject.
+      expect(sql).toContain("Key IN ('ServiceName','SeverityText')");
+      expect(Object.values(params)).not.toContain('ServiceName');
+      expect(Object.values(params)).not.toContain('SeverityText');
+    });
+
+    it('keeps the KV rollup MV query parameter count independent of the number of keys', async () => {
+      const { doMVsAggregateSpy } = setupDefaultLogsSchema();
+      doMVsAggregateSpy.mockResolvedValue(true);
+
+      const manyKeys = Array.from(
+        { length: 80 },
+        (_, i) => `LogAttributes['some.rather.long.attribute.key.${i}']`,
+      );
+      // Route map keys through the MV (not the text index) so they all land in
+      // one batched rollup query.
+      jest
+        .spyOn(metadata, 'getMapColumnTextIndexes')
+        .mockResolvedValue(new Map());
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: manyKeys,
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const params = mvCall![0].query_params as Record<string, string>;
+      // MV table + ColumnIdentifier + limit + 2 timestamps — but never one
+      // param per key.
+      expect(Object.keys(params).length).toBeLessThan(10);
+      const sql = mvCall![0].query as string;
+      expect(sql).toContain("'some.rather.long.attribute.key.0'");
+      expect(sql).toContain("'some.rather.long.attribute.key.79'");
+    });
+
+    it('SQL-escapes inlined rollup keys containing quotes and backslashes', async () => {
+      const { doMVsAggregateSpy } = setupDefaultLogsSchema();
+      doMVsAggregateSpy.mockResolvedValue(true);
+      jest
+        .spyOn(metadata, 'getMapColumnTextIndexes')
+        .mockResolvedValue(new Map());
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ["LogAttributes['it's a key']"],
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const sql = mvCall![0].query as string;
+      // The embedded quote must be escaped, never spliced raw into the IN
+      // list (`'it's a key'` would be an injection / syntax error).
+      expect(sql).toContain("Key IN ('it\\'s a key')");
+      expect(sql).not.toContain("Key IN ('it's a key')");
+    });
+
+    it('applies the time filter to every branch of the KV rollup OR chain', async () => {
+      setupDefaultLogsSchema();
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ['ServiceName', 'SeverityText'],
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const sql = (mvCall![0].query as string).replace(/\s+/g, ' ');
+      // The whole OR chain is wrapped in parens before ANDing the time filter,
+      // otherwise `a OR b AND time` binds the time filter (and notEmpty) to
+      // the last branch only.
+      expect(sql).toMatch(/WHERE \(\(ColumnIdentifier = /);
+      expect(sql).toMatch(/\)\) AND Timestamp >= /);
+    });
+
+    it('inlines map text index key prefixes as escaped literals, not params', async () => {
+      setupDefaultLogsSchema();
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ["LogAttributes['requestId']"],
+      });
+
+      const idxCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('startsWith(token,'),
+      );
+      expect(idxCall).toBeDefined();
+      const sql = idxCall![0].query as string;
+      const params = idxCall![0].query_params as Record<string, string>;
+      expect(sql).toContain("startsWith(token, 'requestId=')");
+      expect(Object.values(params)).not.toContain('requestId=');
     });
 
     it('routes columns without any index or MV entry through the getKeyValues fallback (raw table scan)', async () => {
@@ -1212,11 +1314,14 @@ describe('Metadata', () => {
 
       expect(mapTextIndexCalls.length).toBeGreaterThanOrEqual(2);
 
-      const allParamValues = mapTextIndexCalls.flatMap((c: any[]) =>
-        Object.values(c[0].query_params ?? {}),
-      );
+      // Key prefixes are inlined into the SQL as escaped literals (never as
+      // per-key query params — see the inlining rationale in
+      // getMapTextIndexKeyValues).
+      const allQueries = mapTextIndexCalls
+        .map((c: any[]) => c[0].query as string)
+        .join('\n');
       for (let i = 0; i < keyCount; i++) {
-        expect(allParamValues).toContain(`k${i}=`);
+        expect(allQueries).toContain(`startsWith(token, 'k${i}=')`);
       }
     });
 
diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts
index c443ce58f7..00bb4169ea 100644
--- a/packages/common-utils/src/core/metadata.ts
+++ b/packages/common-utils/src/core/metadata.ts
@@ -1184,9 +1184,20 @@ export class Metadata {
         for (const [columnName, info] of queryOptions.entries()) {
           const orChain = concatChSql(
             ' OR ',
+            // Inline each key prefix as a SQL-escaped literal instead of a
+            // bound query parameter: a filter batch can carry ~100 keys, and
+            // per-key bind params blow past the web client's URL parameter
+            // budget (MAX_URL_BIND_PARAM_LENGTH), silently promoting the
+            // request to a multipart/form-data body that HTTP intermediaries
+            // (e.g. managed query gateways) reject. Inlined literals ride the
+            // POST body with the rest of the SQL. Keys are ingest-controlled
+            // data, so they must be SQL-escaped — `SqlString.escape` returns a
+            // fully-quoted, safely-escaped ClickHouse string literal.
             info.keys.map(
               k =>
-                chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`,
+                chSql`startsWith(token, ${{
+                  UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`),
+                }})`,
             ),
           );
           const partsFilter = await this.partsOverlapFilter({
@@ -1358,17 +1369,29 @@ export class Metadata {
         // this should only be one mv... but we have a for loop in case
         const branch: ChSql[] = [];
         for (const [columnName, keys] of entry) {
-          const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${concatChSql(
-            ',',
-            keys.map(key => chSql`${{ String: key }}`),
-          )}))`;
+          // Inline the keys as SQL-escaped literals instead of one bound
+          // query parameter per key: a filter batch can carry ~100 keys, and
+          // per-key bind params blow past the web client's URL parameter
+          // budget (MAX_URL_BIND_PARAM_LENGTH), silently promoting the
+          // request to a multipart/form-data body that HTTP intermediaries
+          // (e.g. managed query gateways) reject. Inlined literals ride the
+          // POST body with the rest of the SQL. Keys are ingest-controlled
+          // data, so they must be SQL-escaped — `SqlString.escape` returns a
+          // fully-quoted, safely-escaped ClickHouse string literal.
+          const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${{
+            UNSAFE_RAW_SQL: keys.map(key => SqlString.escape(key)).join(','),
+          }}))`;
           branch.push(sql);
         }
+        // The OR chain must be parenthesized as a whole: without the outer
+        // parens, `a OR b AND timeFilter` parses as `a OR (b AND timeFilter)`,
+        // silently dropping the time bound (and notEmpty) from every branch
+        // but the last.
         const sql = chSql`
           SELECT * FROM (
             SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values
             FROM ${tableExpr({ database: databaseName, table: mvName })}
-            WHERE ${concatChSql(' OR ', branch)}
+            WHERE (${concatChSql(' OR ', branch)})
               AND ${timeFilter}
               AND notEmpty(Value)
             GROUP BY ColumnIdentifier, Key

From b021dda26053007a4d126d1bd3ddec6fe9baf043 Mon Sep 17 00:00:00 2001
From: Warren Lee <5959690+wrn14897@users.noreply.github.com>
Date: Tue, 18 Aug 2026 13:20:49 -0700
Subject: [PATCH 2/3] refactor: scope fix to common-utils; leave
 clickhouse-proxy source untouched
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Revert the /clickhouse-proxy body-handling hardening: the multipart bodies
that triggered support-escalation #8482 already pass through the proxy
intact (the manual write of the unparsed body throws and is swallowed, and
the proxy core then pipes the raw unconsumed stream), and the actual fix —
inlining facet keys so the client never emits multipart for these queries —
lives entirely in common-utils.

The integration suite keeps pinning the load-bearing passthrough behavior
(text/plain re-injection, byte-for-byte multipart forwarding) against the
real http-proxy-middleware. The latent proxy bugs the reverted hardening
addressed (charset-suffixed JSON, urlencoded bodies dropped, silent write
failures, unsynced Content-Length) are tracked in #2942.

Changeset narrowed to @hyperdx/common-utils: the api package now carries
test-only changes.
---
 .changeset/fix-filter-facets-behind-proxy.md  |  9 +--
 .../api/__tests__/clickhouseProxy.int.test.ts | 59 +++------------
 .../api/src/routers/api/clickhouseProxy.ts    | 73 +++----------------
 3 files changed, 24 insertions(+), 117 deletions(-)

diff --git a/.changeset/fix-filter-facets-behind-proxy.md b/.changeset/fix-filter-facets-behind-proxy.md
index 2865e8d528..3460a74ba4 100644
--- a/.changeset/fix-filter-facets-behind-proxy.md
+++ b/.changeset/fix-filter-facets-behind-proxy.md
@@ -1,6 +1,5 @@
 ---
 "@hyperdx/common-utils": patch
-"@hyperdx/api": patch
 ---
 
 Fix filter sidebar values disappearing behind query proxies. Batched facet-value
@@ -9,8 +8,6 @@ parameter per key; with ~100 keys this exceeded the ClickHouse web client's URL
 parameter budget, silently promoting the request to a multipart/form-data body
 that proxy gateways can reject — every LowCardinality-column and map-attribute
 filter then vanished without an error. Keys are now inlined as SQL-escaped
-literals so the query rides the POST body. Also fixes an operator-precedence
-bug that applied the rollup time filter to only the last OR branch, and hardens
-the API's /clickhouse-proxy body forwarding (multipart passthrough, charset-
-suffixed JSON content types, urlencoded re-serialization, accurate
-Content-Length).
+literals so the query rides the POST body with a constant parameter count. Also
+fixes an operator-precedence bug that applied the KV rollup time filter (and
+notEmpty guard) to only the last OR branch.
diff --git a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
index 60e719bb55..5f80662f96 100644
--- a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
+++ b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
@@ -17,16 +17,21 @@ type CapturedRequest = {
 
 /**
  * Verifies the /clickhouse-proxy route forwards request bodies to the upstream
- * ClickHouse host byte-for-byte, across every content type the web client
- * emits.
+ * ClickHouse host byte-for-byte for the content types the web client emits.
  *
  * Regression context (ClickHouse support-escalation #8482): when
  * `@clickhouse/client-web` promotes oversized query params to a
  * multipart/form-data body (`use_multipart_params_auto`), no express body
- * parser consumes the stream and the proxy used to attempt
- * `proxyReq.write(req.body)` with the unparsed `{}` placeholder — throwing,
- * swallowing the error, and (depending on the proxy internals) corrupting the
- * upstream request. Filter sidebar facet queries then failed silently.
+ * parser consumes the stream; the proxy's manual `proxyReq.write(req.body)`
+ * throws on the unparsed `{}` placeholder (swallowed), and forwarding works
+ * only because the proxy core then pipes the still-unconsumed raw stream.
+ * These tests pin that load-bearing passthrough plus the parsed text/plain
+ * re-injection path.
+ *
+ * Known-latent cases NOT covered here — `application/json; charset=utf-8` and
+ * `application/x-www-form-urlencoded` bodies are parsed (stream consumed) but
+ * never re-serialized, so the upstream request hangs or arrives body-less.
+ * Tracked in https://github.com/hyperdxio/hyperdx/issues/2942.
  */
 describe('clickhouse-proxy body forwarding', () => {
   const server = getServer();
@@ -100,9 +105,6 @@ describe('clickhouse-proxy body forwarding', () => {
 
     expect(captured).toHaveLength(1);
     expect(captured[0].body.toString()).toBe(sql);
-    expect(captured[0].headers['content-length']).toBe(
-      String(Buffer.byteLength(sql)),
-    );
     // Connection credentials are attached server-side.
     expect(captured[0].headers['x-clickhouse-user']).toBe('default');
   });
@@ -148,45 +150,6 @@ describe('clickhouse-proxy body forwarding', () => {
     );
   });
 
-  it('re-serializes an application/json body with a charset suffix', async () => {
-    const { agent, team } = await getLoggedInAgent(server);
-    const connectionId = await createConnection(team._id.toString());
-
-    const payload = { query: 'SELECT 1', nested: { a: 1 } };
-    await agent
-      .post('/clickhouse-proxy/?query_id=test-json')
-      .set('x-hyperdx-connection-id', connectionId)
-      .set('Content-Type', 'application/json; charset=utf-8')
-      .send(JSON.stringify(payload))
-      .expect(200);
-
-    expect(captured).toHaveLength(1);
-    const forwarded = captured[0].body.toString();
-    expect(JSON.parse(forwarded)).toEqual(payload);
-    // Content-Length must match the re-serialized bytes actually sent, not
-    // the original request's header.
-    expect(captured[0].headers['content-length']).toBe(
-      String(Buffer.byteLength(forwarded)),
-    );
-  });
-
-  it('re-serializes an application/x-www-form-urlencoded body', async () => {
-    const { agent, team } = await getLoggedInAgent(server);
-    const connectionId = await createConnection(team._id.toString());
-
-    await agent
-      .post('/clickhouse-proxy/?query_id=test-urlencoded')
-      .set('x-hyperdx-connection-id', connectionId)
-      .set('Content-Type', 'application/x-www-form-urlencoded')
-      .send('query=SELECT+1&param_foo=bar')
-      .expect(200);
-
-    expect(captured).toHaveLength(1);
-    const forwarded = new URLSearchParams(captured[0].body.toString());
-    expect(forwarded.get('query')).toBe('SELECT 1');
-    expect(forwarded.get('param_foo')).toBe('bar');
-  });
-
   it('rejects requests without a connection id header', async () => {
     const { agent } = await getLoggedInAgent(server);
 
diff --git a/packages/api/src/routers/api/clickhouseProxy.ts b/packages/api/src/routers/api/clickhouseProxy.ts
index f8247844b7..e41946c1b0 100644
--- a/packages/api/src/routers/api/clickhouseProxy.ts
+++ b/packages/api/src/routers/api/clickhouseProxy.ts
@@ -261,71 +261,21 @@ const proxyMiddleware: RequestHandler =
           return res.sendStatus(405);
         }
 
-        // Re-inject the body only when an express body parser consumed the
-        // request stream (json / text / urlencoded). Any other content type —
-        // notably the multipart/form-data bodies @clickhouse/client-web emits
-        // when query params exceed its URL budget (use_multipart_params_auto)
-        // — leaves the stream unconsumed, and the proxy pipes it upstream
-        // verbatim; writing the unparsed `req.body` placeholder here would
-        // corrupt (or previously, error out of) that forwarding.
-        const contentType = _req.headers['content-type'] ?? '';
-        const body = _req.body;
-        let payload: string | Buffer | undefined;
-        if (typeof body === 'string' || Buffer.isBuffer(body)) {
-          // express.text() output (the common case for ClickHouse SQL bodies).
-          payload = body;
-        } else if (contentType.startsWith('application/json')) {
-          // express.json() parsed the stream — re-serialize. startsWith, not
-          // strict equality: clients commonly append `; charset=utf-8`.
+        let body = _req.body;
+        if (_req.headers['content-type'] === 'application/json') {
           try {
-            payload = JSON.stringify(body);
+            body = JSON.stringify(body);
           } catch (e) {
-            console.error('clickhouseProxy failed to serialize JSON body', e);
+            console.error(e);
           }
-        } else if (
-          contentType.startsWith('application/x-www-form-urlencoded') &&
-          body != null &&
-          typeof body === 'object'
-        ) {
-          // express.urlencoded() parsed the stream — re-serialize.
-          const params = new URLSearchParams();
-          for (const [key, value] of Object.entries(body)) {
-            if (Array.isArray(value)) {
-              for (const item of value) params.append(key, String(item));
-            } else {
-              params.append(key, String(value));
-            }
-          }
-          payload = params.toString();
-        } else {
-          // Stream was not consumed by any parser — let it pipe through.
-          return;
-        }
-
-        if (payload == null) {
-          // The stream was consumed but we couldn't reconstruct the body.
-          // Abort loudly instead of forwarding a body-less query upstream,
-          // which surfaces as an opaque 400 from ClickHouse.
-          proxyReq.destroy(new Error('Failed to reconstruct request body'));
-          return;
         }
 
         try {
-          // Re-serialization can change the byte length relative to the
-          // original Content-Length header the proxy forwards; keep them in
-          // sync so the upstream doesn't read a truncated/over-long body.
-          if (!_req.headers['transfer-encoding']) {
-            proxyReq.setHeader('content-length', Buffer.byteLength(payload));
-          }
-          proxyReq.write(payload);
-        } catch (e) {
+          proxyReq.write(body);
+        } catch {
           console.error(
             `clickhouseProxy error writing body, body is type ${typeof body}`,
-            e,
           );
-          // Fail the proxied request rather than silently sending no body;
-          // the error handler below responds to the client.
-          proxyReq.destroy(e instanceof Error ? e : new Error(String(e)));
         }
       },
       proxyRes: (proxyRes, _req, res) => {
@@ -368,13 +318,10 @@ const proxyMiddleware: RequestHandler =
             typeof startedAt === 'number' ? performance.now() - startedAt : 0,
         });
         console.error('Proxy error:', err);
-        const response = _res as Response;
-        if (!response.headersSent) {
-          response.writeHead(500, {
-            'Content-Type': 'application/json',
-          });
-        }
-        response.end(
+        (_res as Response).writeHead(500, {
+          'Content-Type': 'application/json',
+        });
+        _res.end(
           JSON.stringify({
             success: false,
             error: err.message || 'Failed to connect to ClickHouse server',

From 530a0ffe5a82fe9d87b3574f6e6d6eab2cca8a99 Mon Sep 17 00:00:00 2001
From: Warren Lee <5959690+wrn14897@users.noreply.github.com>
Date: Tue, 18 Aug 2026 13:28:02 -0700
Subject: [PATCH 3/3] docs: tighten code comments

---
 packages/api/jest.int.config.js               | 10 +++---
 packages/api/jest.setup.ts                    |  5 ++-
 .../api/__tests__/clickhouseProxy.int.test.ts | 31 ++++++------------
 .../src/__tests__/metadata.test.ts            | 24 +++++---------
 packages/common-utils/src/core/metadata.ts    | 32 ++++++-------------
 5 files changed, 34 insertions(+), 68 deletions(-)

diff --git a/packages/api/jest.int.config.js b/packages/api/jest.int.config.js
index eb316f8991..30584352dd 100644
--- a/packages/api/jest.int.config.js
+++ b/packages/api/jest.int.config.js
@@ -2,12 +2,10 @@ const { createJsWithTsPreset } = require('ts-jest');
 
 const base = require('./jest.config.js');
 
-// http-proxy-middleware v4 (and its proxy core, httpxy) ship ESM-only builds
-// that Jest's CJS loader cannot parse. The unit config blanket-mocks the
-// package (see jest.setup.ts), but the clickhouse-proxy integration tests
-// exercise the real proxy, so the int config transpiles those two packages to
-// CJS via ts-jest instead. `allowJs` lets ts-jest compile their plain-JS ESM
-// sources; the extra `.mjs` transform covers httpxy's `dist/index.mjs` entry.
+// http-proxy-middleware v4 and its deps ship ESM-only builds Jest's CJS
+// loader can't parse. jest.setup.ts blanket-mocks the package; the
+// clickhouse-proxy int tests need the real thing, so transpile it here
+// (`allowJs` for the plain-JS sources, `.mjs` for httpxy's entry).
 const esmDepsTransformCfg = createJsWithTsPreset({
   tsconfig: {
     rootDir: './src',
diff --git a/packages/api/jest.setup.ts b/packages/api/jest.setup.ts
index d7cefbb50f..45d8305646 100644
--- a/packages/api/jest.setup.ts
+++ b/packages/api/jest.setup.ts
@@ -11,9 +11,8 @@ jest.retryTimes(
 );
 
 // http-proxy-middleware v4 is ESM-only and Jest's CJS module loader cannot
-// load ESM packages. Auto-mock it for suites that don't exercise the proxy.
-// The clickhouse-proxy integration tests need the real thing: they opt out
-// via jest.unmock(), and jest.int.config.js transpiles the package to CJS.
+// load ESM packages. Auto-mock it; the clickhouse-proxy int tests opt out via
+// jest.unmock() (jest.int.config.js transpiles the package for them).
 jest.mock('http-proxy-middleware', () => ({
   createProxyMiddleware: jest.fn(() => jest.fn()),
 }));
diff --git a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
index 5f80662f96..b658edfd85 100644
--- a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
+++ b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts
@@ -3,9 +3,8 @@ import http from 'http';
 import { getLoggedInAgent, getServer } from '@/fixtures';
 import Connection from '@/models/connection';
 
-// jest.setup.ts blanket-mocks http-proxy-middleware (ESM-only) for suites that
-// don't touch it; this suite exercises the real proxy, which the int jest
-// config makes loadable by transpiling it (see jest.int.config.js).
+// Use the real proxy instead of the blanket mock in jest.setup.ts; the int
+// jest config transpiles it (see jest.int.config.js).
 jest.unmock('http-proxy-middleware');
 
 type CapturedRequest = {
@@ -16,22 +15,13 @@ type CapturedRequest = {
 };
 
 /**
- * Verifies the /clickhouse-proxy route forwards request bodies to the upstream
- * ClickHouse host byte-for-byte for the content types the web client emits.
+ * Pins /clickhouse-proxy body forwarding: text/plain re-injection and the
+ * raw-stream passthrough of multipart/form-data bodies, which
+ * `@clickhouse/client-web` emits when query params exceed its URL budget
+ * (regression context: ClickHouse support-escalation #8482).
  *
- * Regression context (ClickHouse support-escalation #8482): when
- * `@clickhouse/client-web` promotes oversized query params to a
- * multipart/form-data body (`use_multipart_params_auto`), no express body
- * parser consumes the stream; the proxy's manual `proxyReq.write(req.body)`
- * throws on the unparsed `{}` placeholder (swallowed), and forwarding works
- * only because the proxy core then pipes the still-unconsumed raw stream.
- * These tests pin that load-bearing passthrough plus the parsed text/plain
- * re-injection path.
- *
- * Known-latent cases NOT covered here — `application/json; charset=utf-8` and
- * `application/x-www-form-urlencoded` bodies are parsed (stream consumed) but
- * never re-serialized, so the upstream request hangs or arrives body-less.
- * Tracked in https://github.com/hyperdxio/hyperdx/issues/2942.
+ * Known-latent cases (charset-suffixed JSON, urlencoded) are not covered;
+ * tracked in https://github.com/hyperdxio/hyperdx/issues/2942.
  */
 describe('clickhouse-proxy body forwarding', () => {
   const server = getServer();
@@ -113,9 +103,8 @@ describe('clickhouse-proxy body forwarding', () => {
     const { agent, team } = await getLoggedInAgent(server);
     const connectionId = await createConnection(team._id.toString());
 
-    // Mirror @clickhouse/client-web's use_multipart_params_auto output: a
-    // `query` part plus one `param_*` part per bound parameter, well past the
-    // 4096-byte URL budget that triggers the multipart promotion.
+    // Mirror the client's multipart output: a `query` part plus one
+    // `param_*` part per bound parameter, past the 4096-byte URL budget.
     const boundary = '----clickhouse-js-test-boundary';
     const parts: string[] = [
       `--${boundary}\r\n` +
diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts
index c3b356cede..8be634c4ba 100644
--- a/packages/common-utils/src/__tests__/metadata.test.ts
+++ b/packages/common-utils/src/__tests__/metadata.test.ts
@@ -960,8 +960,7 @@ describe('Metadata', () => {
           );
         });
 
-      // Returned so individual tests can widen/replace the MV-aggregation
-      // answer without re-spying on the private method.
+      // Returned so tests can override the MV-aggregation answer.
       return { doMVsAggregateSpy };
     };
 
@@ -1024,10 +1023,8 @@ describe('Metadata', () => {
       expect(sql).toContain('BY ColumnIdentifier, Key');
       expect(sql).not.toContain('mergeTreeTextIndex(');
       expect(Object.values(params)).toContain('otel_logs_kv_rollup_15m');
-      // Keys are inlined as SQL-escaped literals (NOT bound as query
-      // parameters): one bind param per key blows past the web client's URL
-      // parameter budget for large filter batches, silently promoting the
-      // request to a multipart/form-data body that query gateways reject.
+      // Keys are inlined as escaped literals, not bound as params — per-key
+      // params would push large batches into multipart bodies (see #8482).
       expect(sql).toContain("Key IN ('ServiceName','SeverityText')");
       expect(Object.values(params)).not.toContain('ServiceName');
       expect(Object.values(params)).not.toContain('SeverityText');
@@ -1057,8 +1054,7 @@ describe('Metadata', () => {
       );
       expect(mvCall).toBeDefined();
       const params = mvCall![0].query_params as Record<string, string>;
-      // MV table + ColumnIdentifier + limit + 2 timestamps — but never one
-      // param per key.
+      // A handful of fixed params — never one per key.
       expect(Object.keys(params).length).toBeLessThan(10);
       const sql = mvCall![0].query as string;
       expect(sql).toContain("'some.rather.long.attribute.key.0'");
@@ -1082,8 +1078,7 @@ describe('Metadata', () => {
       );
       expect(mvCall).toBeDefined();
       const sql = mvCall![0].query as string;
-      // The embedded quote must be escaped, never spliced raw into the IN
-      // list (`'it's a key'` would be an injection / syntax error).
+      // The embedded quote must be escaped, never spliced raw.
       expect(sql).toContain("Key IN ('it\\'s a key')");
       expect(sql).not.toContain("Key IN ('it's a key')");
     });
@@ -1101,9 +1096,8 @@ describe('Metadata', () => {
       );
       expect(mvCall).toBeDefined();
       const sql = (mvCall![0].query as string).replace(/\s+/g, ' ');
-      // The whole OR chain is wrapped in parens before ANDing the time filter,
-      // otherwise `a OR b AND time` binds the time filter (and notEmpty) to
-      // the last branch only.
+      // Without parens, `a OR b AND time` binds the time filter to the
+      // last branch only.
       expect(sql).toMatch(/WHERE \(\(ColumnIdentifier = /);
       expect(sql).toMatch(/\)\) AND Timestamp >= /);
     });
@@ -1314,9 +1308,7 @@ describe('Metadata', () => {
 
       expect(mapTextIndexCalls.length).toBeGreaterThanOrEqual(2);
 
-      // Key prefixes are inlined into the SQL as escaped literals (never as
-      // per-key query params — see the inlining rationale in
-      // getMapTextIndexKeyValues).
+      // Key prefixes are inlined as escaped literals, not per-key params.
       const allQueries = mapTextIndexCalls
         .map((c: any[]) => c[0].query as string)
         .join('\n');
diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts
index 00bb4169ea..d9e96e35e0 100644
--- a/packages/common-utils/src/core/metadata.ts
+++ b/packages/common-utils/src/core/metadata.ts
@@ -1184,15 +1184,10 @@ export class Metadata {
         for (const [columnName, info] of queryOptions.entries()) {
           const orChain = concatChSql(
             ' OR ',
-            // Inline each key prefix as a SQL-escaped literal instead of a
-            // bound query parameter: a filter batch can carry ~100 keys, and
-            // per-key bind params blow past the web client's URL parameter
-            // budget (MAX_URL_BIND_PARAM_LENGTH), silently promoting the
-            // request to a multipart/form-data body that HTTP intermediaries
-            // (e.g. managed query gateways) reject. Inlined literals ride the
-            // POST body with the rest of the SQL. Keys are ingest-controlled
-            // data, so they must be SQL-escaped — `SqlString.escape` returns a
-            // fully-quoted, safely-escaped ClickHouse string literal.
+            // Inline keys as SQL-escaped literals, not bind params: ~100
+            // per-key params exceed the web client's URL param budget and
+            // silently switch the request to a multipart body that proxies
+            // may reject. Keys are ingest-controlled, hence SqlString.escape.
             info.keys.map(
               k =>
                 chSql`startsWith(token, ${{
@@ -1369,24 +1364,17 @@ export class Metadata {
         // this should only be one mv... but we have a for loop in case
         const branch: ChSql[] = [];
         for (const [columnName, keys] of entry) {
-          // Inline the keys as SQL-escaped literals instead of one bound
-          // query parameter per key: a filter batch can carry ~100 keys, and
-          // per-key bind params blow past the web client's URL parameter
-          // budget (MAX_URL_BIND_PARAM_LENGTH), silently promoting the
-          // request to a multipart/form-data body that HTTP intermediaries
-          // (e.g. managed query gateways) reject. Inlined literals ride the
-          // POST body with the rest of the SQL. Keys are ingest-controlled
-          // data, so they must be SQL-escaped — `SqlString.escape` returns a
-          // fully-quoted, safely-escaped ClickHouse string literal.
+          // Inline keys as SQL-escaped literals, not bind params: ~100
+          // per-key params exceed the web client's URL param budget and
+          // silently switch the request to a multipart body that proxies
+          // may reject. Keys are ingest-controlled, hence SqlString.escape.
           const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${{
             UNSAFE_RAW_SQL: keys.map(key => SqlString.escape(key)).join(','),
           }}))`;
           branch.push(sql);
         }
-        // The OR chain must be parenthesized as a whole: without the outer
-        // parens, `a OR b AND timeFilter` parses as `a OR (b AND timeFilter)`,
-        // silently dropping the time bound (and notEmpty) from every branch
-        // but the last.
+        // Parenthesize the OR chain: `a OR b AND timeFilter` would bind the
+        // time filter (and notEmpty) to the last branch only.
         const sql = chSql`
           SELECT * FROM (
             SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values