Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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