hyperdxio / hyperdxio/hyperdx

Lucene search on a numeric- or Bool-valued Map column double-escapes the map subscript

Open
#2,765 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

external
Dominant language
TypeScript
Stars
9.9k
Forks
471
Avg merge
2d 4h
Merged PRs (30d)
117

Description

What happens

Searching a Map column whose value type is numeric or Bool generates SQL in which the map subscript is wrapped in an extra layer of backticks, with the inner ones doubled. ClickHouse then reads it as a single identifier literally named `Measures`['latency_ms'] rather than as a map subscript, so the predicate cannot resolve.

The tell is that the same search behaves differently depending only on quoting:

Search Generated predicate
Measures.latency_ms:250 Measures``['latency_ms']` = CAST('250', 'Float64') ``` ❌
Measures.latency_ms:"250" `Measures`['latency_ms'] = CAST('250', 'Float64')
Measures.latency_ms:>250 `Measures`['latency_ms'] > '250'
-Measures.latency_ms:250 Measures``['latency_ms']` != CAST('250', 'Float64') ``` ❌
Flags.cached:true Flags``['cached']` = 1 ``` ❌
Flags.cached:"true" Flags``['cached']` = 1 ``` ❌

For a Map(String, Bool) column both the quoted and unquoted forms are affected, so there is no working spelling of a Bool-map search. Map(String, String) columns are unaffected (Attributes.host:web1`Attributes`['host'] ILIKE '%web1%'), as are plain non-map columns.

Reproduced against main @ e73af381.

Cause

Three call sites pass the already-rendered column expression as a SqlString ?? placeholder, which applies escapeId to it a second time:

  • packages/common-utils/src/queryParser.ts:502SQLSerializer.eq, Bool branch
  • packages/common-utils/src/queryParser.ts:1384CustomSchemaSQLSerializerV2.fieldSearch, Bool branch
  • packages/common-utils/src/queryParser.ts:1396CustomSchemaSQLSerializerV2.fieldSearch, Number branch

getColumnForField() returns column already rendered — `Measures`['latency_ms'] for a map key, and the bare name for a plain column. Every other branch interpolates it raw: eq/Number (:514), gte (:597), lte, lt, gt, range (:731) and the final ILIKE (:1554). Those three are the odd ones out.

git log -L shows :514 used ${column} and :1396 used ?? from the initial common-utils commit (6ee29abe) — the paths look copy-pasted and then diverged, rather than deliberately different.

Why the tests don't catch it

queryParser.test.ts:2735 (falls back for Map(String, Float64) value type) uses the quoted form NumericAttributes.count:"42", which routes to the correct eq/Number path, and asserts only not.toContain('has(') / toContain('CAST'). No test uses an unquoted numeric-map term, and no test uses a Map(String, Bool) column at all.

Repro

Drop this in packages/common-utils/src/__tests__/ and run yarn jest --ci zzprobe:

import { ClickhouseClient } from '@/clickhouse/node';
import { getMetadata } from '@/core/metadata';
import { CustomSchemaSQLSerializerV2, SearchQueryBuilder } from '@/queryParser';

describe('probe', () => {
  function buildSerializer() {
    const metadata = getMetadata(new ClickhouseClient({ host: 'http://localhost:8123' }));
    metadata.getColumn = jest.fn().mockImplementation(async ({ column }) => {
      if (column === 'Measures') return { name: 'Measures', type: 'Map(String, Float64)' };
      if (column === 'Flags') return { name: 'Flags', type: 'Map(String, Bool)' };
      if (column === 'Body') return { name: 'Body', type: 'String' };
      return undefined;
    });
    metadata.getMaterializedColumnsLookupTable = jest.fn().mockImplementation(async () => new Map());
    metadata.getColumns = jest.fn().mockImplementation(async () => [
      { name: 'Measures', type: 'Map(String, Float64)', default_type: '', default_expression: '' },
      { name: 'Flags', type: 'Map(String, Bool)', default_type: '', default_expression: '' },
    ]);
    metadata.getSkipIndices = jest.fn().mockImplementation(async () => []);
    metadata.getSetting = jest.fn().mockImplementation(async () => '0');
    metadata.getServerVersion = jest.fn().mockImplementation(async () => [26, 5, 0, 0] as const);
    return new CustomSchemaSQLSerializerV2({
      metadata,
      databaseName: 'default',
      tableName: 'otel_logs',
      connectionId: 'test',
      implicitColumnExpression: 'Body',
    });
  }

  it.each([
    'Measures.latency_ms:250',
    'Measures.latency_ms:"250"',
    'Flags.cached:true',
  ])('probe %s', async q => {
    process.stdout.write(`\n${q}\n  ${await new SearchQueryBuilder(q, buildSerializer()).build()}\n`);
    expect(1).toBe(1);
  });
});
Two possible fixes — I'd rather you picked

I have a patch ready either way, but the choice is yours to make because they differ in an observable way:

A. Match the sibling branches — swap ?? for ${column} at the three sites. Smallest possible diff and makes all branches consistent. But for plain (non-map) columns those three branches currently get their backticks from escapeId, so this drops them: IsError:true would go from `IsError` = 1 to IsError = 1. That matches what the ILIKE, gt/lt and range branches already emit today (ServiceName ILIKE '%foo%', unquoted), so it is consistent — but it does remove quoting that is currently there, which would bite a column named e.g. order.

B. Escape only when it is a bare column name — keep escapeId for plain columns and interpolate raw only for a rendered map expression (mapKeyIndexExpression is already available in fieldSearch as the discriminator). Preserves today's quoting for plain columns; slightly larger diff, and leaves the serializer inconsistent about quoting overall.

There is also a C — have getColumnForField() return a uniformly-escaped expression and interpolate raw everywhere — which is the tidiest end state but a much wider change.

Happy to open a PR with whichever you prefer, with tests covering unquoted numeric-map, negated numeric-map, and Bool-map searches in both directions. Just say which.


Found while working on #2764. This report was AI-assisted, per the AI-Assisted Development section of CONTRIBUTING.md; every SQL string above was executed, not inferred.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

The affected code is in packages/common-utils/src/queryParser.ts at SQLSerializer.eq and CustomSchemaSQLSerializerV2.fieldSearch; start by reading the three ?? call sites alongside the sibling interpolation branches. Extend packages/common-utils/src/tests/queryParser.test.ts around the Map(String, Float64) case with unquoted and negated numeric-map searches plus Bool-map searches. Done means the generated predicates no longer double-escape map subscripts and the relevant Jest tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
clickhouse, typescript
Domain
backend, databases, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.