Skip to content

fix(common-utils): guard metadata rollup queries against invalid date… - #2941

Open
Vansh98789 wants to merge 6 commits into
hyperdxio:mainfrom
Vansh98789:fix/invalid-date-metadata-rollup
Open

fix(common-utils): guard metadata rollup queries against invalid date…#2941
Vansh98789 wants to merge 6 commits into
hyperdxio:mainfrom
Vansh98789:fix/invalid-date-metadata-rollup

Conversation

@Vansh98789

@Vansh98789 Vansh98789 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes BAD_QUERY_PARAMETER (457) failures on metadata key/value rollup queries (e.g. filter dropdowns and field lists).

When a metadata query is built with an invalid date in its date range, Date.getTime() returns NaN. That NaN is serialized as the literal string "nan" and bound to a ClickHouse Int64 parameter, causing the query to fail with:

Code: 457. DB::Exception: Cannot parse 'nan' as Int64

This happens in two places in metadata.ts:

  • getMapKeys — builds rollup queries from getAlignedDateRange(dateRange, granularity) and binds { Int64: date.getTime() }
  • getMetadataMVKeyValues — same pattern for batched key/value lookups

Changes

File Change
common-utils/src/core/utils.ts Added isDateRangeValid(dateRange) — returns false if either bound's getTime() is not a finite number
common-utils/src/core/metadata.ts Added guard in getMapKeys — skips metadata queries and returns [] when the date range is invalid
common-utils/src/core/metadata.ts Added guard in getMetadataMVKeyValues — returns undefined so callers fall through to the raw-table strategy
common-utils/src/core/__tests__/metadata.test.ts Added unit tests for the helper and getMapKeys guard
.changeset/tidy-invalid-dates.md Added patch changeset

Tests

  • isDateRangeValid accepts a valid range
  • isDateRangeValid rejects a range with an invalid start date
  • isDateRangeValid rejects a range with an invalid end date
  • getMapKeys returns [] and never calls ClickHouse when given an invalid range

Test command

cd packages/common-utils
npx jest src/core/__tests__/metadata.test.ts

Screenshots or video

N/A — non-UI change.

How to test on Vercel preview

N/A — non-UI change.

References

@changeset-bot

changeset-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7e7736a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hyperdx/common-utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

@Vansh98789 is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR prevents non-finite or out-of-range URL timestamps from becoming invalid dates and adds guards around metadata rollup queries.

  • Adds a bounded epoch-millisecond URL parser for shared time-range state.
  • Adds reusable date-range validation and applies it before metadata rollup queries.
  • Adds unit coverage and a patch changeset for the common-utils behavior.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/timeQuery.ts Replaces the generic float parser with a finite, Date-range-bounded epoch parser for URL-backed time state.
packages/common-utils/src/core/metadata.ts Adds invalid-date guards before metadata rollup query construction and expands diagnostic context.
packages/common-utils/src/core/utils.ts Adds a shared helper that validates both date-range bounds as finite timestamps.
packages/common-utils/src/core/tests/metadata.test.ts Covers date-range validation and verifies that getMapKeys avoids ClickHouse for invalid dates.
.changeset/tidy-invalid-dates.md Records the common-utils metadata-query fix as a patch release.

Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/invalid-dat..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: 5 files vs base b52a6fa8 — guards metadata rollup queries against Invalid Date ranges (getMapKeys, getMetadataMVKeyValues), adds isDateRangeValid, and fixes the root cause in packages/app/src/timeQuery.ts by rejecting non-finite/out-of-range from/to URL params at parse time.

✅ No critical issues found. The core fix is sound: MAX_VALID_EPOCH_MS boundary is correct, serialize matches nuqs's default so URL round-trips are unchanged, and getMetadataMVKeyValues returning undefined is safely filtered out of the Promise.allSettled aggregation (metadata.ts:2457).

🟡 P2 -- recommended

  • .changeset/tidy-invalid-dates.md:2 -- The diff changes @hyperdx/app behavior in timeQuery.ts (swapping parseAsFloat for parseAsValidEpochMs), but the changeset only declares @hyperdx/common-utils, which is absent from the fixed group [api, app, otel-collector], so the app-side parser change ships with no version bump or changelog entry.
    • Fix: Add "@hyperdx/app": patch to the changeset frontmatter so the app behavior change is released and recorded.
    • project-standards
  • packages/app/src/timeQuery.ts:418 -- parseAsValidEpochMs is the actual root-cause fix for the invalid-date bug yet has no test asserting it rejects Infinity (e.g. 1e400), NaN, and out-of-range magnitudes while accepting the ±8_640_000_000_000_000 boundary and normal epoch values.
    • Fix: Export the parser (or its parse fn) and add unit tests for the rejection and boundary cases plus a parse/serialize round-trip.
    • testing, correctness, maintainability, kieran-typescript, previous-comments
  • packages/common-utils/src/core/metadata.ts:1349 -- The new getMetadataMVKeyValues invalid-range guard (returns undefined) has no test, unlike the sibling getMapKeys guard which does; the MV rollup path reached via getKeyValues (line 2411) is uncovered.
    • Fix: Add a test driving the batched MV path with an invalid dateRange, asserting the ClickHouse query fn is never called.
    • testing, correctness
🔵 P3 nitpicks (5)
  • packages/app/src/timeQuery.ts:419 -- parse: (v) => wraps a single arrow param in parens, violating the repo's arrowParens: "avoid" prettier setting (line 426 correctly omits them); eslint-plugin-prettier will fail make ci-lint.
    • Fix: Change to parse: v => and run yarn lint:fix.
  • packages/common-utils/src/core/utils.ts:1081 -- isDateRangeValid dereferences dateRange[0].getTime() without guarding nullish elements, while the adjacent warn strings use dateRange[0]?.toString(), implying elements can be nullish; a type-violating JS caller passing [undefined, undefined] would throw before the guard returns.
    • Fix: Either drop the redundant ?. in the warn messages or harden the check with an instanceof Date test to keep the type and runtime assumptions in agreement.
    • correctness, kieran-typescript
  • packages/common-utils/src/core/metadata.ts:667 -- The invalid-date console.warn pattern (message + call-site context + new Error(...).stack) is duplicated across getMapKeys and getMetadataMVKeyValues.
    • Fix: Optionally extract a small helper that formats the warning while each caller keeps its own context fields and return sentinel; only worthwhile if a third call site appears.
  • packages/common-utils/src/core/metadata.ts:673 -- The return [] on an invalid range still produces a silently empty result (e.g. empty filter dropdown), the concern the maintainer raised on the prior review thread; traceability (call-site context + stack trace) and one known producer are now addressed, but the silent-empty behavior was intentionally kept.
    • Fix: Confirm with the maintainer whether the traceable-warning approach closes the thread, or surface the invalid range to the caller instead of masking it.
    • previous-comments, correctness
  • .changeset/tidy-invalid-dates.md:5 -- The changeset file lacks a trailing newline whereas sibling changesets have one; lint-staged runs prettier only on code files, not .md, so this is a convention nit.
    • Fix: Add a trailing newline.

Reviewers (6): correctness, testing, maintainability, project-standards, kieran-typescript, previous-comments.

Testing gaps:

  • No test exercises timeQuery.ts at all; the URL-param root-cause fix is verified only by reasoning.
  • No test covers a mixed range (one valid + one invalid Date) through getMapKeys.

Residual risk (out of scope for this diff): the URL-parser fix only covers the from/to time-range params; other parseAsFloat-backed params (sfrom/sto, latencyMin/latencyMax, heatmap xMin/yMin, etc.) remain able to parse 1e400Infinity → Invalid Date, though none currently feed the metadata rollup path directly.

'getMapKeys: skipping metadata queries, dateRange contains an invalid date',
dateRange,
);
return [];

@wrn14897 wrn14897 Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to return [] here, since that would produce falsy results. For this ticket, the intent is to figure out what in the app is generating the invalid date range.

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.

@wrn14897 Traced the root cause: parseAsFloat in timeQuery.ts only rejects NaN, not Infinity or out-of-range values. A URL like ?from=1e400 parses to Infinity, which becomes new Date(Infinity) = Invalid Date downstream - that's what's producing the "nan" Int64 param.

Two changes pushed to this branch:

  1. timeQuery.ts - added a parser that rejects non-finite/out-of-range from/to values at the URL-parsing stage, so the Invalid Date can't be constructed from this path anymore.
  2. metadata.ts - kept the return [] guard as-is per your concern, but added call-site context (database/table/column) and a stack trace to the warning, so if it ever fires again, it's traceable back to the caller instead of silent.

Let me know if there's another producer you've seen in the query_log this doesn't cover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid Date reaches metadata rollup queries as "nan" Int64 param (BAD_QUERY_PARAMETER 457)

3 participants