Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/v2-search-subsecond-start-max-results.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@hyperdx/common-utils': patch
'@hyperdx/api': patch
---

fix: Floor the time filter start to the precision of a coarse timestamp column (DateTime, DateTime64 below ms) when a finer column also filters the range, so a sub-second start no longer drops the rest of that tick; make the external API v2 search `maxResults` cap configurable via `EXTERNAL_API_SEARCH_MAX_RESULTS` (defaults to 2000, the previous hardcoded value)
39 changes: 39 additions & 0 deletions packages/api/src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,45 @@ describe('config', () => {
);
});

describe('EXTERNAL_API_SEARCH_MAX_RESULTS', () => {
const ORIGINAL = process.env.EXTERNAL_API_SEARCH_MAX_RESULTS;

afterEach(() => {
if (ORIGINAL === undefined) {
delete process.env.EXTERNAL_API_SEARCH_MAX_RESULTS;
} else {
process.env.EXTERNAL_API_SEARCH_MAX_RESULTS = ORIGINAL;
}
jest.resetModules();
});

const loadMaxResults = () => {
let loaded!: typeof import('@/config');
jest.isolateModules(() => {
loaded = jest.requireActual<typeof import('@/config')>('@/config');
});
return loaded.EXTERNAL_API_SEARCH_MAX_RESULTS;
};

it('defaults to 2000 when unset', () => {
delete process.env.EXTERNAL_API_SEARCH_MAX_RESULTS;
expect(loadMaxResults()).toBe(2000);
});

it('accepts a valid positive integer', () => {
process.env.EXTERNAL_API_SEARCH_MAX_RESULTS = '10000';
expect(loadMaxResults()).toBe(10_000);
});

it.each(['abc', '0', '50', '-5', '100rows', '', '0.5', 'Infinity'])(
'falls back to 2000 for %p',
raw => {
process.env.EXTERNAL_API_SEARCH_MAX_RESULTS = raw;
expect(loadMaxResults()).toBe(2000);
},
);
});

describe('EXPRESS_SESSION_SECRET', () => {
const ORIGINAL_SECRET = process.env.EXPRESS_SESSION_SECRET;
const ORIGINAL_LOCAL_APP_MODE = process.env.IS_LOCAL_APP_MODE;
Expand Down
11 changes: 11 additions & 0 deletions packages/api/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,17 @@ export const EXTERNAL_API_RATE_LIMIT_MAX = (() => {
return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : 100;
})();

export const EXTERNAL_API_SEARCH_DEFAULT_MAX_RESULTS = 100;

export const EXTERNAL_API_SEARCH_MAX_RESULTS = (() => {
const parsed = Number(env.EXTERNAL_API_SEARCH_MAX_RESULTS);
// A cap below the default would make requests without maxResults invalid.
return Number.isSafeInteger(parsed) &&
parsed >= EXTERNAL_API_SEARCH_DEFAULT_MAX_RESULTS
? parsed
: 2000;
})();

// FOR CI ONLY
export const CLICKHOUSE_HOST = env.CLICKHOUSE_HOST as string;
export const CLICKHOUSE_USER = env.CLICKHOUSE_USER as string;
Expand Down
12 changes: 9 additions & 3 deletions packages/api/src/routers/external-api/v2/search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import express from 'express';
import { ObjectId } from 'mongodb';
import { z } from 'zod';

import {
EXTERNAL_API_SEARCH_DEFAULT_MAX_RESULTS,
EXTERNAL_API_SEARCH_MAX_RESULTS,
} from '@/config';
import { parseTimeRange } from '@/mcp/tools/query/helpers';
import { getNonNullUserWithTeam } from '@/middleware/auth';
import { processRequestWithEnhancedErrors as validateRequest } from '@/utils/enhancedErrors';
Expand Down Expand Up @@ -223,10 +227,12 @@ const searchRequestSchema = z.object({
.number()
.int()
.min(1)
.max(2000)
.max(EXTERNAL_API_SEARCH_MAX_RESULTS)
Comment thread
pulpdrew marked this conversation as resolved.
Resolved
.optional()
.default(100)
.describe('Maximum rows to return (1-2000). Default: 100.'),
.default(EXTERNAL_API_SEARCH_DEFAULT_MAX_RESULTS)
.describe(
`Maximum rows to return (1-${EXTERNAL_API_SEARCH_MAX_RESULTS}). Default: ${EXTERNAL_API_SEARCH_DEFAULT_MAX_RESULTS}.`,
),
offset: z
.number()
.int()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2924,4 +2924,79 @@ describe('queryChartConfig Integration Tests', () => {
});
});
});

// Mirrors the OTel logs schema, where a DateTime column precedes the
// DateTime64(9) column in the timestamp expression for primary-key pruning.
describe('sub-second start on a DateTime + DateTime64 source', () => {
const LOGS_TABLE = 'logs_subsecond_start_int_test';
const start = new Date('2026-10-13T22:03:21.827Z');
const end = new Date('2026-10-13T22:05:00Z');

const searchConfig = (
dateRange: [Date, Date],
dateRangeEndInclusive = true,
): ChartConfigWithOptDateRange => ({
connection: 'test-connection',
from: { databaseName: DATABASE, tableName: LOGS_TABLE },
select: 'Body',
where: '',
whereLanguage: 'sql',
timestampValueExpression: 'TimestampTime, Timestamp',
orderBy: 'Timestamp ASC',
dateRange,
dateRangeEndInclusive,
});

const bodies = async (config: ChartConfigWithOptDateRange) => {
const result = await hdxClient.queryChartConfig({
config,
metadata,
querySettings: undefined,
});
return (result.data as Array<{ Body: string }>).map(r => r.Body);
};

beforeAll(async () => {
await client.command({
query: `CREATE OR REPLACE TABLE ${DATABASE}.${LOGS_TABLE} (
Timestamp DateTime64(9, 'UTC'),
TimestampTime DateTime('UTC') DEFAULT toDateTime(Timestamp),
Body String
)
ENGINE = MergeTree
ORDER BY (TimestampTime, Timestamp)`,
});
await client.insert({
table: `${DATABASE}.${LOGS_TABLE}`,
values: [
{ Timestamp: '2026-10-13 22:03:21.500000000', Body: 'before-start' },
{ Timestamp: '2026-10-13 22:03:21.900000000', Body: 'same-second' },
{ Timestamp: '2026-10-13 22:03:22.100000000', Body: 'next-second' },
],
format: 'JSONEachRow',
});
});

afterAll(async () => {
await client.command({
query: `DROP TABLE IF EXISTS ${DATABASE}.${LOGS_TABLE}`,
});
});

it('returns rows in the rest of the start second', async () => {
expect(await bodies(searchConfig([start, end]))).toEqual([
'same-second',
'next-second',
]);
});

it('does not return the same row from the preceding window', async () => {
const previousWindow = searchConfig(
[new Date('2026-10-13T22:00:00Z'), start],
false,
);

expect(await bodies(previousWindow)).toEqual(['before-start']);
});
});
});
185 changes: 184 additions & 1 deletion packages/common-utils/src/__tests__/renderChartConfig.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2650,7 +2650,9 @@ describe('renderChartConfig', () => {
mockMetadata.getColumn.mockImplementation(async ({ column }) =>
column === 'date'
? ({ type: 'Date' } as ColumnMeta)
: ({ type: 'DateTime' } as ColumnMeta),
: column === 'TimestampTime'
? ({ type: 'DateTime' } as ColumnMeta)
: ({ type: 'DateTime64(3)' } as ColumnMeta),
);
});

Expand Down Expand Up @@ -2759,6 +2761,187 @@ describe('renderChartConfig', () => {
);
});

it.each([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Sub-second fix and configurable cap are only checked as SQL strings, never against ClickHouse or the route

Add a case to packages/api/src/routers/external-api/__tests__/search.int.test.ts. That file already inserts rows with millisecond Timestamps. Insert rows at e.g. .800/.900 of a second and search with a .500 startTime. Assert both rows come back, which is the exact production bug in the PR description. Also add a route-level check that maxResults above 2000 is accepted when EXTERNAL_API_SEARCH_MAX_RESULTS is raised. Right now only the parsed constant is tested in config.test.ts, so a regression in how ClickHouse compares the floored bound, or in how the zod .max() is wired, would still pass.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The integration logs table only has a full-precision Timestamp, with no DateTime column, so a route-level test of the fix would need its own table schema in the test. The cap is read when the module loads, so testing a raised cap would mean reloading the router with the env var set. I verified the ClickHouse comparison by hand (DateTime >= DateTime64 bound is false within the same second; flooring makes it true). The SQL shape is covered by unit tests, and the existing 2000 boundary tests still cover the default cap.

['inclusive', true],
['exclusive', false],
])(
'floors a sub-second %s start on a DateTime column and keeps it inclusive',
async (_, dateRangeStartInclusive) => {
const start = new Date('2026-10-13T22:03:21.827Z');
const end = new Date('2026-10-13T22:05:00.500Z');

const actual = await timeFilterExpr({
timestampValueExpression: 'TimestampTime, Timestamp',
dateRangeEndInclusive: false,
dateRangeStartInclusive,
dateRange: [start, end],
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
});

const precise = dateRangeStartInclusive ? '>=' : '>';
expect(parameterizedQueryToSql(actual)).toBe(
`(TimestampTime >= fromUnixTimestamp64Milli(${new Date('2026-10-13T22:03:21Z').getTime()}) AND TimestampTime < fromUnixTimestamp64Milli(${end.getTime()}))AND(Timestamp ${precise} fromUnixTimestamp64Milli(${start.getTime()}) AND Timestamp < fromUnixTimestamp64Milli(${end.getTime()}))`,
);
},
);

it.each([
['inclusive', true, '>='],
['exclusive', false, '>'],
])(
'leaves a %s start unfloored on a DateTime-only source so adjacent windows do not overlap',
async (_, dateRangeStartInclusive, op) => {
const start = new Date('2026-10-13T22:03:21.827Z');
const end = new Date('2026-10-13T22:05:00Z');

const actual = await timeFilterExpr({
timestampValueExpression: 'TimestampTime',
dateRangeEndInclusive: true,
dateRangeStartInclusive,
dateRange: [start, end],
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
});

expect(parameterizedQueryToSql(actual)).toBe(
`(TimestampTime ${op} fromUnixTimestamp64Milli(${start.getTime()}) AND TimestampTime <= fromUnixTimestamp64Milli(${end.getTime()}))`,
);
},
);

it.each([
['DateTime', 'toDateTime($__fromTime_ms)'],
['DateTime64(1)', 'toDateTime64($__fromTime_ms, 1)'],
])(
'floors the start macro on a %s column in a raw SQL template',
async (type, expectedStart) => {
mockMetadata.getColumn.mockImplementation(async ({ column }) =>
column === 'coarse'
? ({ type } as ColumnMeta)
: ({ type: 'DateTime64(9)' } as ColumnMeta),
);

const actual = await timeFilterExpr({
timestampValueExpression: 'coarse, Timestamp',
dateRangeEndInclusive: true,
dateRangeStartInclusive: true,
dateRange: [
new Date('2026-10-13T22:03:21.827Z'),
new Date('2026-10-13T22:05:00Z'),
],
isRenderingRawSqlTemplate: true,
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
});

expect(parameterizedQueryToSql(actual)).toBe(
`(coarse >= ${expectedStart} AND coarse <= $__toTime_ms)AND(Timestamp >= $__fromTime_ms AND Timestamp <= $__toTime_ms)`,
);
},
);

it.each([
['DateTime', '22:03:21.000'],
["DateTime('UTC')", '22:03:21.000'],
['Nullable(DateTime)', '22:03:21.000'],
['DateTime64(0)', '22:03:21.000'],
["DateTime64(0, 'UTC')", '22:03:21.000'],
['DateTime64(1)', '22:03:21.800'],
['DateTime64(2)', '22:03:21.820'],
['DateTime64(3)', '22:03:21.827'],
])(
'floors the start bound of a %s column to %s when a finer column exists',
async (type, flooredStart) => {
mockMetadata.getColumn.mockImplementation(async ({ column }) =>
column === 'coarse'
? ({ type } as ColumnMeta)
: ({ type: 'DateTime64(9)' } as ColumnMeta),
);
const start = new Date('2026-10-13T22:03:21.827Z');
const end = new Date('2026-10-13T22:05:00Z');

const actual = await timeFilterExpr({
timestampValueExpression: 'coarse, Timestamp',
dateRangeEndInclusive: true,
dateRangeStartInclusive: true,
dateRange: [start, end],
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
});

expect(parameterizedQueryToSql(actual)).toBe(
`(coarse >= fromUnixTimestamp64Milli(${new Date(`2026-10-13T${flooredStart}Z`).getTime()}) AND coarse <= fromUnixTimestamp64Milli(${end.getTime()}))AND(Timestamp >= fromUnixTimestamp64Milli(${start.getTime()}) AND Timestamp <= fromUnixTimestamp64Milli(${end.getTime()}))`,
);
},
);

describe('sub-second start flooring edge cases', () => {
const start = new Date('2026-10-13T22:03:21.827Z');
const flooredStart = new Date('2026-10-13T22:03:21Z');
const end = new Date('2026-10-13T22:05:00Z');

const render = (
timestampValueExpression: string,
includedDataInterval?: string,
) =>
timeFilterExpr({
timestampValueExpression,
dateRangeEndInclusive: true,
dateRangeStartInclusive: false,
dateRange: [start, end],
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
includedDataInterval,
}).then(parameterizedQueryToSql);

it('does not floor when the finer column type cannot be resolved', async () => {
mockMetadata.getColumn.mockImplementation(async ({ column }) =>
column === 'TimestampTime'
? ({ type: 'DateTime' } as ColumnMeta)
: undefined,
);

expect(await render('TimestampTime, Timestamp')).toBe(
`(TimestampTime > fromUnixTimestamp64Milli(${start.getTime()}) AND TimestampTime <= fromUnixTimestamp64Milli(${end.getTime()}))AND(Timestamp > fromUnixTimestamp64Milli(${start.getTime()}) AND Timestamp <= fromUnixTimestamp64Milli(${end.getTime()}))`,
);
});

it('does not treat a toStartOf column as the finer column', async () => {
expect(await render('toStartOfMinute(Timestamp), TimestampTime')).toBe(
`(toStartOfMinute(Timestamp) >= toStartOfMinute(fromUnixTimestamp64Milli(${start.getTime()})) AND toStartOfMinute(Timestamp) <= toStartOfMinute(fromUnixTimestamp64Milli(${end.getTime()})))AND(TimestampTime > fromUnixTimestamp64Milli(${start.getTime()}) AND TimestampTime <= fromUnixTimestamp64Milli(${end.getTime()}))`,
);
});

it('floors the start inside the includedDataInterval expansion', async () => {
expect(await render('TimestampTime, Timestamp', '1 minute')).toBe(
`(TimestampTime >= toStartOfInterval(fromUnixTimestamp64Milli(${flooredStart.getTime()}), INTERVAL 1 minute) - INTERVAL 1 minute AND TimestampTime <= toStartOfInterval(fromUnixTimestamp64Milli(${end.getTime()}), INTERVAL 1 minute) + INTERVAL 1 minute)AND(Timestamp > toStartOfInterval(fromUnixTimestamp64Milli(${start.getTime()}), INTERVAL 1 minute) - INTERVAL 1 minute AND Timestamp <= toStartOfInterval(fromUnixTimestamp64Milli(${end.getTime()}), INTERVAL 1 minute) + INTERVAL 1 minute)`,
);
});

it('treats a Nullable(DateTime64(9)) column as the finer column', async () => {
mockMetadata.getColumn.mockImplementation(async ({ column }) =>
column === 'TimestampTime'
? ({ type: 'DateTime' } as ColumnMeta)
: ({ type: 'Nullable(DateTime64(9))' } as ColumnMeta),
);

expect(await render('TimestampTime, Timestamp')).toBe(
`(TimestampTime >= fromUnixTimestamp64Milli(${flooredStart.getTime()}) AND TimestampTime <= fromUnixTimestamp64Milli(${end.getTime()}))AND(Timestamp > fromUnixTimestamp64Milli(${start.getTime()}) AND Timestamp <= fromUnixTimestamp64Milli(${end.getTime()}))`,
);
});
});

it.each(['Date32', 'Nullable(Date)', 'LowCardinality(Date)'])(
'wraps a %s column in toDate() and keeps the bounds inclusive',
async type => {
Expand Down
Loading
Loading