Skip to content

fix: v2 search drops rows after a sub-second startTime; make maxResults cap configurable #3374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vinzee wants to merge 1 commit into from
Closed

fix: v2 search drops rows after a sub-second startTime; make maxResults cap configurable #3374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/configurable-search-max-results.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperdx/api': patch
---

feat: Make the external API v2 search `maxResults` cap configurable via `EXTERNAL_API_SEARCH_MAX_RESULTS`. Defaults to 2000, matching the previous hardcoded value.
5 changes: 5 additions & 0 deletions .changeset/datetime-subsecond-start.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperdx/common-utils': patch
---

fix: Floor the time filter start on second-precision DateTime columns so a sub-second start no longer drops the rest of that second
3 changes: 1 addition & 2 deletions packages/api/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4239,9 +4239,8 @@
"maxResults": {
"type": "integer",
"minimum": 1,
"maximum": 2000,
"default": 100,
"description": "Maximum number of rows to return. Default is 100, max is 2000."
"description": "Maximum number of rows to return. Default is 100, max is 2000\nunless the server sets EXTERNAL_API_SEARCH_MAX_RESULTS.\n"
},
"offset": {
"type": "integer",
Expand Down
39 changes: 39 additions & 0 deletions packages/api/src/__tests__/config.test.ts
View file
Open in desktop
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', '-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
5 changes: 5 additions & 0 deletions packages/api/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ export const EXTERNAL_API_RATE_LIMIT_MAX = (() => {
return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : 100;
})();

export const EXTERNAL_API_SEARCH_MAX_RESULTS = (() => {
const parsed = Number(env.EXTERNAL_API_SEARCH_MAX_RESULTS);
return Number.isSafeInteger(parsed) && parsed > 0 ? 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: 8 additions & 4 deletions packages/api/src/routers/external-api/v2/search.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import express from 'express';
import { ObjectId } from 'mongodb';
import { z } from 'zod';

import { 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 @@ -117,9 +118,10 @@ const CH_USER_INPUT_ERRORS = new Set([
* maxResults:
* type: integer
* minimum: 1
* maximum: 2000
* default: 100
* description: Maximum number of rows to return. Default is 100, max is 2000.
* description: |
* Maximum number of rows to return. Default is 100, max is 2000
* unless the server sets EXTERNAL_API_SEARCH_MAX_RESULTS.
* offset:
* type: integer
* minimum: 0
Expand Down Expand Up @@ -223,10 +225,12 @@ const searchRequestSchema = z.object({
.number()
.int()
.min(1)
.max(2000)
.max(EXTERNAL_API_SEARCH_MAX_RESULTS)

@greptile-apps greptile-apps Bot Oct 15, 2026

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.

P1 Default searches fail

If EXTERNAL_API_SEARCH_MAX_RESULTS is below 100, requests that omit maxResults fail. The configuration accepts values such as 50, but Zod inserts the default of 100 and checks it against the configured maximum. Clients relying on the documented default receive a 400 response.

Use Math.min(100, EXTERNAL_API_SEARCH_MAX_RESULTS) as the default and add a request test for a cap below 100.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@vinzee vinzee Oct 15, 2026

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.

Fixed in 979ab720c: the default is now Math.min(100, EXTERNAL_API_SEARCH_MAX_RESULTS). I didn't add a request test. The cap is read when the module loads, so a test would need to reload the router with the env var set, and the schema isn't exported. Happy to add one if you prefer a particular pattern.

.optional()
.default(100)
.describe('Maximum rows to return (1-2000). Default: 100.'),
.describe(
`Maximum rows to return (1-${EXTERNAL_API_SEARCH_MAX_RESULTS}). Default: 100.`,
),
offset: z
.number()
.int()
Expand Down
57 changes: 56 additions & 1 deletion packages/common-utils/src/__tests__/renderChartConfig.test.ts
View file
Open in desktop
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,59 @@ describe('renderChartConfig', () => {
);
});

it.each([
['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(['DateTime', "DateTime('UTC')", 'Nullable(DateTime)'])(
'floors the start bound of a %s column',
async type => {
mockMetadata.getColumn.mockImplementation(
async () => ({ type }) 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: 'ts',
dateRangeEndInclusive: true,
dateRangeStartInclusive: true,
dateRange: [start, end],
connectionId: 'test-connection',
databaseName: 'default',
tableName: 'target_table',
metadata: mockMetadata,
});

expect(parameterizedQueryToSql(actual)).toBe(
`(ts >= fromUnixTimestamp64Milli(${new Date('2026-10-13T22:03:21Z').getTime()}) AND ts <= 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
20 changes: 14 additions & 6 deletions packages/common-utils/src/core/renderChartConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,13 +992,21 @@ export async function timeFilterExpr({
);
}

const columnType = stripTypeWrappers(columnMeta?.type ?? '');
// DateTime truncates to the second, so a sub-second or exclusive start
// compared as-is drops the rest of that second.
const isSecondPrecision = /^DateTime(?:\(.*\))?$/i.test(columnType);
const columnStartTime = isSecondPrecision
? Math.floor(startTime / 1000) * 1000
: startTime;

const rawStartBound = isRenderingRawSqlTemplate
? includedDataInterval
? chSql`toStartOfInterval($__fromTime_ms, INTERVAL $__interval_s second) - INTERVAL $__interval_s second`
: chSql`$__fromTime_ms`
: includedDataInterval
? chSql`toStartOfInterval(fromUnixTimestamp64Milli(${{ Int64: startTime }}), INTERVAL ${includedDataInterval}) - INTERVAL ${includedDataInterval}`
: chSql`fromUnixTimestamp64Milli(${{ Int64: startTime }})`;
? chSql`toStartOfInterval(fromUnixTimestamp64Milli(${{ Int64: columnStartTime }}), INTERVAL ${includedDataInterval}) - INTERVAL ${includedDataInterval}`
: chSql`fromUnixTimestamp64Milli(${{ Int64: columnStartTime }})`;
Comment on lines +1008 to +1009

@greptile-apps greptile-apps Bot Oct 15, 2026

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.

P2 Converted charts lose rows

The new rounding only applies to queries with concrete dates. With isRenderingRawSqlTemplate, a DateTime column still compares against $__fromTime_ms, which expands to the original millisecond bound. Converting a chart using TimestampTime, Timestamp to SQL therefore drops the first partial second again, while the builder now keeps it.

Apply the same rounding in the generated template and cover the conversion with a test.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@vinzee vinzee Oct 15, 2026

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.

Fixed in 979ab720c: in template mode a DateTime column now compares against toDateTime($__fromTime_ms). Added a template test; the builderToRawSql snapshots that use a DateTime timestamp were updated to match.


const rawEndBound = isRenderingRawSqlTemplate
? includedDataInterval
Expand All @@ -1016,13 +1024,13 @@ export async function timeFilterExpr({
? chSql`${toStartOf.function}(${rawEndBound}${toStartOf.formattedRemainingArgs})`
: rawEndBound;

const isDateType =
/^Date(?:32)?$/i.test(stripTypeWrappers(columnMeta?.type ?? '')) ||
isToDateExpr;
const isDateType = /^Date(?:32)?$/i.test(columnType) || isToDateExpr;

// toStartOf* and Date filters must stay inclusive — strict < on a rounded value drops a whole interval
const startOp =
dateRangeStartInclusive || toStartOf || isDateType ? '>=' : '>';
dateRangeStartInclusive || toStartOf || isDateType || isSecondPrecision

@github-actions github-actions Bot Oct 15, 2026

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.

🟠 major — Exclusive start becomes inclusive on DateTime-only sources, so adjacent windows return the same rows twice

The forced >= exists so the coarse column doesn't drop rows that the precise column (e.g. Timestamp) would still match. When the DateTime column is the only entry in timestampValueExpression, nothing checks the exact bound, and dateRangeStartInclusive: false turns into ts >= floor(start). DBTraceWaterfallChart.tsx:401-413 splits a trace at focusDate into a before window [start, focus] (end defaults to inclusive) and an after window with an exclusive start. For a DateTime-only source, rows in focus's second now appear in both windows. Before this PR, ts > focus kept them only in the before window. Fix: force >= on a DateTime column only when another column in valueExpressions keeps the exact bound. Otherwise keep the caller's operator. Then add a single-column test with dateRangeStartInclusive: false; the new it.each(['DateTime', ...]) test only covers inclusive starts.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

@vinzee vinzee Oct 15, 2026

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.

Fixed in 979ab720c. The forced >= now applies only when another column in the expression is finer than a second and still enforces the exact bound. A DateTime-only source keeps the caller's >. Flooring is a no-op there, because ts > floor(start) and ts > start match the same whole-second values. Added a single-column exclusive-start test.

? '>='
: '>';
Comment on lines +1031 to +1033

@greptile-apps greptile-apps Bot Oct 15, 2026

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.

P1 Boundary rows repeat

If a source has only one DateTime timestamp column, this changes an exclusive whole-second start from > to >=. Rows exactly at the boundary now return even when dateRangeStartInclusive is false. useEventsAroundFocus uses that flag to separate its before and after queries, then joins their rows without removing duplicates.

Keep the exact comparison for a standalone timestamp. Only relax a coarse filter when another column enforces the requested boundary.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@vinzee vinzee Oct 15, 2026

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.

Same fix as the thread above (979ab720c): >= is forced only when a finer column keeps the exact bound, so a DateTime-only source keeps its exclusive start.

@greptile-apps greptile-apps Bot Oct 15, 2026

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.

Thanks — I agree that the multi-column TimestampTime, Timestamp case is safe: the precise Timestamp predicate still enforces the requested bound, so my duplicate-row concern does not apply there.

However, the current condition does not actually make >= conditional on a finer column. isSecondPrecision is computed for the current column and directly ORed into startOp (renderChartConfig.ts:1031-1033). For a DateTime-only source with an exclusive start of 22:03:21.827, columnStartTime is floored to 22:03:21.000 and the generated predicate becomes DateTime >= ..., admitting rows before .827.

To make the stated invariant true, the code needs to detect that another finer timestamp column exists and only relax the DateTime predicate in that case; otherwise the standalone DateTime path should retain >.

@vinzee vinzee Oct 15, 2026

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.

That review ran on ca7d9b2; the PR was closed before the fix push, so it doesn't show 979ab720c. On the branch, hasPreciseColumn is computed across all columns first, and >= is forced only when isSecondPrecision && hasPreciseColumn. A DateTime-only source keeps > (see the "keeps an exclusive start exclusive on a DateTime-only source" test).

const endOp =
dateRangeEndInclusive || toStartOf || isDateType ? '<=' : '<';

Expand Down
Loading