Skip to content

handle more nullable types#1984

Open
MikeShi42 wants to merge 3 commits intomainfrom
mikeshi/handle-more-nullable-types
Open

handle more nullable types#1984
MikeShi42 wants to merge 3 commits intomainfrom
mikeshi/handle-more-nullable-types

Conversation

@MikeShi42
Copy link
Contributor

@MikeShi42 MikeShi42 commented Mar 25, 2026 •
edited by Copilot AI
Loading

Summary

Fix ClickHouse query error when expanding log rows with Nullable(DateTime64) columns (and other Nullable types).

  • The convertCHDataTypeToJSType function didn't generically unwrap Nullable(...) types, so Nullable(DateTime64(...)) fell through to the default string comparison instead of using parseDateTime64BestEffort()
  • Added general Nullable(...) recursive unwrapping (matching the existing LowCardinality(...) pattern)
  • Hoisted null value handling above the type switch in processRowToWhereClause so all column types (Date, Array, Map, etc.) correctly emit isNull() for null values

Screenshots or video

N/A — no UI changes.

How to test locally or on Vercel

  1. Set up a ClickHouse table with a Nullable(DateTime64) column and ingest some rows (including rows with null values in that column).
  2. Open the log explorer and expand a row that has a Nullable(DateTime64) column.
  3. Verify that clicking into the row no longer returns a 400 error.
  4. Verify that clicking into a row where the Nullable(DateTime64) column is null correctly filters using isNull().

References

  • Related PRs:

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.
@MikeShi42 MikeShi42 requested review from a team and wrn14897 and removed request for a team March 25, 2026 06:47
@changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2026 •
edited
Loading

🦋 Changeset detected

Latest commit: 11ee222

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@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
@vercel
Copy link

vercel bot commented Mar 25, 2026 •
edited
Loading

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 26, 2026 4:55am

Request Review
@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2026 •
edited
Loading

Knip - Unused Code Analysis

⚪ No changes detected (182 issues on both main and PR)

What is this?

Knip finds unused files, dependencies, and exports in your codebase.
This comment compares the PR branch against main to detect regressions.

Run yarn knip locally to see full details.
@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2026 •
edited
Loading

PR Review

✅ No critical issues found.

The two fixes are correct and follow existing patterns:

  • Nullable( unwrapping in convertCHDataTypeToJSType mirrors the existing LowCardinality( pattern with correct slice offsets (9 chars)
  • Null check hoisted above the switch is a clean fix that correctly unifies handling across all types
  • Tests added for both changes; coverage looks adequate
@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2026 •
edited
Loading

E2E Test Results

✅ All tests passed • 102 passed • 3 skipped • 1082s

Status Count
✅ Passed 102
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →
@MikeShi42 MikeShi42 changed the title Mikeshi/handle more nullable types handle more nullable types Mar 25, 2026
@MikeShi42
Copy link
Contributor Author

MikeShi42 commented Mar 25, 2026

@copilot can you pls fix lint errors
Copy link

Copilot AI commented Mar 25, 2026

@copilot can you pls fix lint errors

Fixed in 877d593 — sorted named imports alphabetically and corrected prettier formatting (double quotes for strings containing single quotes, and .toBe() call placement).
@MikeShi42
Copy link
Contributor Author

MikeShi42 commented Mar 25, 2026

okay @claude maybe you can help me fix this lint issue...
@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2026 •
edited
Loading

Claude finished @MikeShi42's task in 57s —— View job

Fixed lint issues

  • Read relevant files
  • Fix dead code in useRowWhere.tsx — removed unreachable value == null || (null already handled at line 58) and updated stale comment
  • Remove redundant explicit Nullable(Int/UInt/Float/String) checks in clickhouse/index.ts — superseded by the generic Nullable() unwrapper
  • Committed and pushed (ede119c5)
@MikeShi42 MikeShi42 force-pushed the mikeshi/handle-more-nullable-types branch from 9ab50bc to 11ee222 Compare March 26, 2026 04:52
MikeShi42
MikeShi42 commented Mar 26, 2026
View reviewed changes
AGENTS.md

**Tools handle formatting and linting automatically** via pre-commit hooks.
Focus on implementation; don't manually format code.
**After finishing all code edits**, run `yarn lint:fix` to auto-fix formatting
Copy link
Contributor Author

@MikeShi42 MikeShi42 Mar 26, 2026

Choose a reason for hiding this comment

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

after losing the war with agents vs linter, I'm telling agents to always lint - I'm not sure why we did not encourage this anyways? @teeohhem maybe you can lmk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge

2 participants

@MikeShi42