Skip to content

feat: Add RawSqlChartConfig types for SQL-based Table#1846

Open
pulpdrew wants to merge 5 commits intomainfrom
drew/raw-sql-chart-config
Open

feat: Add RawSqlChartConfig types for SQL-based Table#1846
pulpdrew wants to merge 5 commits intomainfrom
drew/raw-sql-chart-config

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Mar 4, 2026

Summary

This PR is the first step towards raw SQL-driven charts.

  • It introduces updated ChartConfig types, which are now unions of BuilderChartConfig (which is unchanged from the current ChartConfig typesandRawSqlChartConfig` types which represent sql-driven charts.
  • It adds very basic support for SQL-driven tables in the Chart Explorer and Dashboard pages. This is currently behind a feature toggle and enabled only in preview environments and for local development.

The changes in most of the files in this PR are either type updates or the addition of type guards to handle the new ChartConfig union type.

The DBEditTimeChartForm has been updated significantly to (a) add the Raw SQL option to the table chart editor and (b) handle conversion from internal form state (which can now include properties from either branch of the ChartConfig union) to valid SavedChartConfigs (which may only include properties from one branch).

Significant changes are in:

  • packages/app/src/components/ChartEditor/types.ts
  • packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx
  • packages/app/src/components/ChartEditor/utils.ts
  • packages/app/src/components/DBEditTimeChartForm.tsx
  • packages/app/src/components/DBTableChart.tsx
  • packages/app/src/components/SQLEditor.tsx
  • packages/app/src/hooks/useOffsetPaginatedQuery.tsx

Future PRs will add templating to the Raw SQL driven charts for date range and granularity injection; support for other chart types driven by SQL; improved placeholder, validation, and error states; and improved support in the external API and import/export.

Screenshots or video

Screen.Recording.2026-03-04.at.4.45.58.PM.mov

How to test locally or on Vercel

The SQL-driven table can be tested in the preview environment or locally.

References

  • Linear Issue: HDX-3580
  • Related PRs:

@vercel
Copy link

vercel bot commented Mar 4, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 5, 2026 4:29pm

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2026

🦋 Changeset detected

Latest commit: 66f5240

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/api Patch
@hyperdx/app 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

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 45s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Gather context and read key changed files
  • Review for critical issues

Overall: This is a well-structured refactor adding RawSqlChartConfig as a discriminated union alongside the existing BuilderChartConfig. The type narrowing via guards.ts is clean and the separation of concerns is good.

A few items worth addressing:

  • ⚠️ renderChartConfig uses UNSAFE_RAW_SQL for raw SQL execution (packages/common-utils/src/core/renderChartConfig.ts, line ~1407) — the sqlTemplate value is passed directly with no sanitization. This is expected for "raw SQL" mode but ensure there's proper authorization on the API side (team-scoped access, only authenticated users can execute arbitrary SQL). If any API endpoint accepts configType: 'sql' configs from external/untrusted sources without sufficient auth, that's a SQL injection risk.

  • ⚠️ convertFormStateToChartConfig in ChartEditor/utils.ts (line ~88) — when form.configType === 'sql' but form.displayType !== DisplayType.Table, it falls through to the if (source) branch and builds a BuilderChartConfig with configType: 'sql' not stripped. The omit(form, ['series', 'configType', 'sqlTemplate']) handles this, but the if (form.configType === 'sql') guard at the top only checks for 'sql' type (not display type), while convertFormStateToSavedChartConfig additionally checks displayType === DisplayType.Table. These two functions have inconsistent conditions — the chart config converter will return a RawSqlChartConfig for any display type with configType: 'sql', while the saved config converter only does it for Table. Intentional? If Raw SQL is only for tables, both should check display type.

  • ⚠️ Pagination silently disabled for raw SQL (useOffsetPaginatedQuery.tsx, line ~101) — getNextPageParam returns undefined for raw SQL configs, meaning only the first page of results is shown. The comment says "Pagination is not supported" but there's no user-facing indication of this limitation. Consider showing a message or limit indicator in DBTableChart.

  • ℹ️ isBuilderChartConfig is a negative check (guards.ts, line ~18) — return !isRawSqlChartConfig(chartConfig) means any config without configType === 'sql' is considered a builder config. If new config types are added later, they'd incorrectly be treated as builder configs. Consider using a positive discriminant check for robustness.

  • ℹ️ ChartEditorFormState is a wide partial intersection (ChartEditor/types.ts) — Partial<BuilderSavedChartConfig> & Partial<RawSqlSavedChartConfig> means connection appears in both union members but with different semantics (builder uses connection from _ChartConfigSchema while raw SQL uses it explicitly). This could cause subtle issues if connection handling differs. Worth a comment explaining the merge is intentional.

✅ No critical blocking issues — the type architecture is sound and the narrowing guards are consistently applied across ~65 files. The security consideration around UNSAFE_RAW_SQL is the most important item to verify is properly authorized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

E2E Test Results

All tests passed • 84 passed • 3 skipped • 968s

Status Count
✅ Passed 84
❌ Failed 0
⚠️ Flaky 4
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew force-pushed the drew/raw-sql-chart-config branch from d4507d5 to b49707e Compare March 4, 2026 19:54
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 1m 5s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Reading key changed files
  • Reviewing for critical issues

Overall the discriminated union approach is clean and well-executed. A few items worth addressing:

  • ⚠️ Inconsistent configType === 'sql' guard between converters (ChartEditor/utils.ts:48,90) — convertFormStateToSavedChartConfig only returns RawSqlSavedChartConfig when displayType === DisplayType.Table, but convertFormStateToChartConfig converts to RawSqlChartConfig for any display type with configType === 'sql'. This means a raw SQL config for a non-table display type would render as raw SQL but save as a builder config — mismatched round-trip behavior. Fix this →

  • ⚠️ No user-visible indicator for disabled pagination (useOffsetPaginatedQuery.tsx:104-107 + DBTableChart.tsx) — Raw SQL tables silently show only the first page. Users have no indication their results may be truncated. Add a note/banner in DBTableChart when isRawSqlChartConfig(config) is true. Fix this →

  • ℹ️ isBuilderChartConfig is a negative check (guards.ts:20) — return !isRawSqlChartConfig(chartConfig) treats any config without configType === 'sql' as a builder config. If a 3rd config type is added later, it would silently be treated as a builder config. Consider using a positive discriminant (e.g. checking for required builder fields like timestampValueExpression).

  • ℹ️ alert injected into raw SQL tiles (dashboard.ts:107-109) — getDashboard unconditionally spreads alert from the alerts map into every tile config, including raw SQL tiles which don't have an alert field in their schema. This doesn't break anything since the value is undefined most of the time, but it's worth being consistent — e.g. only attach alert when isBuilderSavedChartConfig(t.config).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 39s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Reading key changed files
  • Reviewing for critical issues

✅ The discriminated union approach is clean and the isBuilderSavedChartConfig guard in dashboard.ts is now correctly applied (previous concern about alert injection is resolved — pickAlertsByTile only looks at builder configs).

A few remaining items:

  • ⚠️ Inconsistent configType === 'sql' guard between converters (ChartEditor/utils.ts:48,90) — convertFormStateToSavedChartConfig only returns RawSqlSavedChartConfig when displayType === DisplayType.Table, but convertFormStateToChartConfig at line 90 converts to RawSqlChartConfig for any display type with configType === 'sql'. A raw SQL config for a non-table display type would render as raw SQL but save as a builder config — mismatched round-trip behavior. Fix this →

  • ⚠️ Pagination silently disabled for raw SQL (useOffsetPaginatedQuery.tsx:104-107) — getNextPageParam returns undefined for raw SQL, but DBTableChart.tsx shows no user-visible indication that results may be truncated. Consider adding a note when isRawSqlChartConfig(config) is true. Fix this →

  • ℹ️ isBuilderChartConfig is a negative check (guards.ts:20) — return !isRawSqlChartConfig(chartConfig) means any future 3rd config type would silently be treated as a builder config. Low risk now, but worth noting for future extensibility.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 27s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Reading key changed files
  • Reviewing for critical issues

✅ The discriminated union approach is well-structured and consistently applied across the codebase. No critical blocking issues found.

One remaining item from prior reviews:

  • ⚠️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:19,30) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as a builder config, potentially causing runtime errors. Low risk now but worth a positive discriminant check (e.g. 'timestampValueExpression' in chartConfig) for forward-safety. Fix this →

return result;
};

const convertChartConfigToExternalChartSeries = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions were unused, easier to delete them now than update the types.

@pulpdrew pulpdrew force-pushed the drew/raw-sql-chart-config branch from 8f9f741 to fa4e6a2 Compare March 4, 2026 20:31
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 1m 0s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Reading key changed files
  • Reviewing for critical issues

✅ The discriminated union approach is clean and consistently applied. Previous review concerns about inconsistent guards in convertFormStateToChartConfig vs convertFormStateToSavedChartConfig have been resolved — both now correctly gate on configType === 'sql' && displayType === DisplayType.Table.

One remaining actionable item:

  • ⚠️ Pagination silently truncates results for raw SQL (useOffsetPaginatedQuery.tsx:104-106) — getNextPageParam returns undefined for raw SQL configs, so hasNextPage is always false. But DBTableChart.tsx shows no indication that results may be cut off. Users won't know they're only seeing a partial result set. Consider adding a note below the table when isRawSqlChartConfig(config) is true — e.g. "Showing first page of results. Pagination is not supported for raw SQL charts." Fix this →

ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:20,32) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as builder config. Low risk for this PR but worth noting before the union grows.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 41s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Reading key changed files
  • Reviewing for critical issues

✅ The discriminated union approach is clean and well-executed. Type guards in guards.ts are consistently applied across ~65 files. Previous concerns about inconsistent configType === 'sql' guards between convertFormStateToChartConfig and convertFormStateToSavedChartConfig have been resolved — both now correctly gate on configType === 'sql' && displayType === DisplayType.Table.

Two remaining items:

  • ⚠️ Pagination silently truncates results (useOffsetPaginatedQuery.tsx:104) — getNextPageParam returns undefined for raw SQL, but DBTableChart.tsx shows no indication results may be cut off. Users won't know they're only seeing a partial result set. Consider adding a note below the table when isRawSqlChartConfig(config) is true. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:19,30) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as a builder config, potentially causing runtime errors. Low risk now, but worth noting before the union grows.

@pulpdrew pulpdrew force-pushed the drew/raw-sql-chart-config branch from 502a599 to 29250e8 Compare March 4, 2026 21:06
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 27s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

✅ The discriminated union approach is clean and well-executed. Previous concerns about inconsistent guards between converters are resolved — both convertFormStateToChartConfig and convertFormStateToSavedChartConfig correctly gate on configType === 'sql' && displayType === DisplayType.Table.

One remaining item:

  • ⚠️ Pagination silently truncates results (useOffsetPaginatedQuery.tsx:104) — getNextPageParam returns undefined for raw SQL, so hasNextPage is always false. DBTableChart.tsx shows no indication that results are capped. The tableBottom prop only renders when hasNextPage is true (line 240), so users get no feedback. Add a note when isRawSqlChartConfig(config) is true — e.g. alongside the tableBottom condition. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:20,32) — any future 3rd config type would silently be treated as builder config. Low risk for this PR but worth a comment noting this assumption.

@pulpdrew pulpdrew marked this pull request as ready for review March 4, 2026 21:48
@pulpdrew pulpdrew requested a review from knudtty March 4, 2026 21:56
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude finished @pulpdrew's task in 31s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

✅ The discriminated union approach is clean and well-structured. Type guards in guards.ts are consistently applied across the codebase. Previous review concerns about inconsistent guards have been resolved.

Two remaining items:

  • ⚠️ Pagination silently truncates raw SQL results (DBTableChart.tsx:239-245) — tableBottom only renders when hasNextPage is true, but for raw SQL configs hasNextPage is always false (useOffsetPaginatedQuery.tsx:105). Users have no indication that results may be truncated at the page limit. Add a note in the tableBottom area when isRawSqlChartConfig(config) is true. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig use negative checks (guards.ts:20,32) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as builder. Low risk for this PR, but consider a positive discriminant (e.g. checking 'timestampValueExpression' in chartConfig) before the union grows.

@pulpdrew pulpdrew force-pushed the drew/raw-sql-chart-config branch from c1e88c9 to d5e5a82 Compare March 5, 2026 01:08
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Claude finished @pulpdrew's task in 32s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

✅ The discriminated union approach is clean and well-structured. Type guards in guards.ts are consistently applied across the codebase.

Two remaining items carried over from prior reviews:

  • ⚠️ Pagination silently truncates raw SQL results (DBTableChart.tsx:239-244) — tableBottom only renders when hasNextPage is true, but for raw SQL hasNextPage is always false (useOffsetPaginatedQuery.tsx:105). Users get no indication results may be cut off at the page limit. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:20,32) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as a builder config. Low risk now but worth noting before the union grows.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Claude finished @pulpdrew's task in 43s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Gather context and read key changed files
  • Review for critical issues

✅ The discriminated union approach is well-structured and consistently applied. Previous concerns about inconsistent guards between convertFormStateToChartConfig and convertFormStateToSavedChartConfig are resolved — both correctly gate on configType === 'sql' && displayType === DisplayType.Table.

One remaining actionable item:

  • ⚠️ Pagination silently truncates raw SQL results (DBTableChart.tsx:239-244) — tableBottom only renders when hasNextPage is true, but for raw SQL hasNextPage is always false (useOffsetPaginatedQuery.tsx:105). Users get no indication results may be cut off at the page limit. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:20,32) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as a builder config. Low risk for this PR but worth noting before the union grows.

@pulpdrew pulpdrew force-pushed the drew/raw-sql-chart-config branch from 2e08b42 to fb5531a Compare March 5, 2026 13:26
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Claude finished @pulpdrew's task in 40s —— View job


PR Review: feat: Add RawSqlChartConfig types for SQL-based Table

  • Gather context and read key changed files
  • Review for critical issues

✅ The discriminated union approach is well-structured and the type guards in guards.ts are consistently applied across the codebase.

One remaining actionable item that has persisted across all prior reviews:

  • ⚠️ Pagination silently truncates raw SQL results (DBTableChart.tsx:239-244) — tableBottom only renders when hasNextPage is true, but for raw SQL hasNextPage is always false (useOffsetPaginatedQuery.tsx:105). Users get no indication their results may be cut off at the page limit. Fix this →

  • ℹ️ isBuilderChartConfig / isBuilderSavedChartConfig are negative checks (guards.ts:20,32) — return !isRawSqlChartConfig(...) means any future 3rd config type would silently be treated as builder. Low risk now but worth noting before the union grows.

return [];
}

if (isRawSqlSavedChartConfig(tile.config)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to support this eventually? If so, should we add a comment with a linked issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no immediate plans to support this.

Comment on lines +1064 to +1066
onChange={(value: string) => {
onChange(value == 'sql' ? 'sql' : 'builder');
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be simplified

Suggested change
onChange={(value: string) => {
onChange(value == 'sql' ? 'sql' : 'builder');
}}
onChange={onChange}


import resizeStyles from '@/../styles/ResizablePanel.module.scss';

export default function RawSqlChartEditor({
Copy link
Contributor

Choose a reason for hiding this comment

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

If it doesn't exist, we should create a follow up issue that makes it really easy to use a daterange from a date picker. Like a macro replacement in your SQL query or something. It's much more natural to use a date picker to select times than dealing with timestamps and typing out WHERE Timestamp > toDateTime(0023187932719) AND Timestamp < toDateTime(00348391784931)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I do see the datetime there, but it's in a weird spot. I feel it should be inline with the connection selector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also some UX indicator that the date range is being applied would probably be helpful. Even a "disable date range" button that changes an icon, similar to accelerated symbols for materialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree - already working on it in this followup PR

/>
</Group>
<Box style={{ position: 'relative' }}>
<SQLEditorControlled
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not currently line wrap, but should
(query for reference: select toStartOfMinute(Timestamp) as minuteslice, SeverityText, count() as severity_count from otel_logs WHERE Timestamp > toDateTime(1772722398) - INTERVAL 15 MINUTE AND Timestamp < toDateTime(1772722398) group by minuteslice, SeverityText ORDER BY minuteslice DESC)

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added line wrapping


import resizeStyles from '@/../styles/ResizablePanel.module.scss';

export default function RawSqlChartEditor({
Copy link
Contributor

Choose a reason for hiding this comment

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

I can run truncate otel_logs and delete all data in the table. We should have some parsing at least to ensure we are only running SELECT queries

**/
const SharedChartDisplaySettingsSchema = z.object({
displayType: z.nativeEnum(DisplayType).optional(),
numberFormat: NumberFormatSchema.optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We will eventually want more granular number formatting I think

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, though this also applies to non-raw-sql based charts. Created an issue 👍

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.

2 participants