Skip to content

Comments

fix: searching json number property error#1767

Merged
fleon merged 2 commits intomainfrom
karl/fix-search-json-number
Feb 20, 2026
Merged

fix: searching json number property error#1767
fleon merged 2 commits intomainfrom
karl/fix-search-json-number

Conversation

@karl-power
Copy link
Contributor

@karl-power karl-power commented Feb 20, 2026

Fixes #1238
Fixes HDX-2551

Checks if a property is a number or boolean and chooses the appropriate JSON extract function.

@vercel
Copy link

vercel bot commented Feb 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Feb 20, 2026 4:29pm

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2026

🦋 Changeset detected

Latest commit: e0a8d59

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector 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

@github-actions
Copy link
Contributor

PR Review: fix: searching json number property error

✅ No critical issues found.

Minor observations:

  • ⚠️ Integer valuesJSONExtractFloat is used for all numbers, but integers could use JSONExtractInt for more precise matching. Edge case: a field with value 42 would be searched as a float (42.0), which may cause mismatches depending on how ClickHouse handles type coercion in the query.
  • ⚠️ Copy/search actions (lines 258 and 349) don't pass jsonFunction → only the "filter" action gets the number-aware function; copy and other actions fall back to JSONExtractString. This is probably fine for copy behavior, but worth confirming search consistency across all action types.

Otherwise clean fix — good refactor of the function name and appropriate use of the default parameter.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

PR Review: fix: searching json number property error

  • ⚠️ Chart action still uses JSONExtractString for number values (line 360-364) → The chart action is gated by typeof value === 'number' but doesn't pass JSONExtractFloat to buildJSONExtractQuery. Same root bug as the search case — charting a numeric JSON property will be broken. Should apply the same fix there.

  • ℹ️ JSONExtractFloat used for all numbers including integers → For integer JSON values, JSONExtractInt would be more precise; JSONExtractFloat works for typical cases but may lose precision on large integers (>2^53).

Otherwise the fix is clean — the filter action correctly remains string-only so the JSONExtractString default there is fine.

@karl-power karl-power force-pushed the karl/fix-search-json-number branch from db1eef5 to 347542f Compare February 20, 2026 14:41
@karl-power karl-power requested review from a team and fleon and removed request for a team February 20, 2026 14:41
fleon
fleon previously approved these changes Feb 20, 2026
@fleon
Copy link
Contributor

fleon commented Feb 20, 2026

Would be nice to add some tests for this function.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

Claude Code Review

  • ⚠️ JSONExtractFloat used for all numeric types → Consider JSONExtractInt for integer values to avoid precision loss on large integers (e.g. IDs, timestamps). JSONExtractFloat (Float64) cannot represent all 64-bit integers exactly.
  • ⚠️ Path segments are interpolated without escaping single quotes → Keys containing apostrophes would produce malformed SQL. Escape with p.replace(/'/g, "''"). (Pre-existing issue, but worth fixing here since this code is being touched.)

✅ Logic is correct, type detection approach is sound, and test coverage is thorough.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

Claude Code Review

  • ⚠️ JSONExtractFloat used for all numeric types → Consider JSONExtractInt for integer values to avoid precision loss on large integers (e.g. IDs, timestamps). JSONExtractFloat (Float64) cannot represent all 64-bit integers exactly.
  • ⚠️ Path segments are interpolated without escaping single quotes → Keys containing apostrophes would produce malformed SQL. Escape with p.replace(/'/g, "''"). (Pre-existing issue, but worth fixing here since this code is being touched.)
  • ⚠️ Chart action (line ~358) still uses default JSONExtractString for numeric values → If charting numeric JSON fields is supported, apply the same type-detection logic there.

✅ Logic is correct, type detection approach is sound, and test coverage is thorough.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

Claude Code Review

  • ⚠️ JSONExtractFloat used for all numeric types → Consider JSONExtractInt for integer values to avoid precision loss on large integers (e.g. IDs, timestamps). JSONExtractFloat (Float64) cannot represent all 64-bit integers exactly.
  • ⚠️ Path segments are interpolated without escaping single quotes → Keys containing apostrophes would produce malformed SQL. Escape with p.replace(/'/g, "''"). (Pre-existing issue, but worth fixing here since this code is being touched.)

✅ Logic is correct, type detection approach is sound, and test coverage is thorough.

@github-actions
Copy link
Contributor

E2E Test Results

All tests passed • 70 passed • 4 skipped • 837s

Status Count
✅ Passed 70
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 4

Tests ran across 4 shards in parallel.

View full report →

@fleon fleon merged commit 38286f6 into main Feb 20, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

filter by parsed JSON string feature - fails when value is not a string

2 participants