Skip to content

feat: add custom ORDER BY expression for Log and Trace sources#1856

Open
dhable wants to merge 5 commits intomainfrom
dan/hdx-3577-option-to-customize-order-by-for-sources
Open

feat: add custom ORDER BY expression for Log and Trace sources#1856
dhable wants to merge 5 commits intomainfrom
dan/hdx-3577-option-to-customize-order-by-for-sources

Conversation

@dhable
Copy link
Contributor

@dhable dhable commented Mar 5, 2026

Allow users to configure a custom ORDER BY clause per source that overrides the default auto-computed ordering on the search page. This supports deployments where the default sort order doesn't align with the table's ordering keys (e.g. disabled read-in-order optimization).

Made-with: Cursor

Summary

Screenshots or video

Before After

How to test locally or on Vercel

References

  • Linear Issue:
  • Related PRs:

Allow users to configure a custom ORDER BY clause per source that
overrides the default auto-computed ordering on the search page.
This supports deployments where the default sort order doesn't align
with the table's ordering keys (e.g. disabled read-in-order optimization).

Made-with: Cursor
@vercel
Copy link

vercel bot commented Mar 5, 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 8:34pm

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2026

🦋 Changeset detected

Latest commit: 865cc8e

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 5, 2026

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


PR Review

  • Read changed files
  • Analyze code quality and potential issues
  • Post review findings

No critical issues found.

The implementation is clean and well-tested. A few minor observations:

  • ⚠️ orderByExpression accepts arbitrary SQL with no syntax validation — an invalid expression will silently break search results. Consider adding a test query or EXPLAIN validation similar to what useExplainQuery does for other fields. Fix this →

  • ℹ️ The field is only exposed in the UI for Log and Trace sources, but SourceBaseSchema includes it for all source kinds. If this is intentional (Metric/Session sources don't need it), that's fine — just noting the asymmetry.

  • ℹ️ The e2e cleanup step resets to orderByExpression: '' (empty string) rather than removing the field entirely. The truthy check if (source.orderByExpression) correctly handles this, so behavior is correct.

@dhable dhable requested a review from pulpdrew March 5, 2026 19:38
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

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


PR Review

No critical issues found.

The implementation is clean — schema, model, UI, and query logic are all consistent. A few minor observations:

  • ⚠️ orderByExpression accepts arbitrary SQL with no syntax validation — an invalid expression will silently produce broken search results. Consider using useExplainQuery (already imported in SourceForm.tsx) to warn on invalid syntax, similar to other validated fields.
  • ℹ️ The field lives in SourceBaseSchema (applies to all source kinds) but UI is only shown for Log and Trace — if Metric/Session sources ever call getOrderBy, the expression would be picked up silently. This appears intentional but worth documenting.
  • ℹ️ The e2e cleanup resets orderByExpression: '' (empty string) — the truthy check if (source.orderByExpression) in DBSearchPage.tsx handles this correctly.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

E2E Test Results

All tests passed • 85 passed • 3 skipped • 973s

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

Tests ran across 4 shards in parallel.

View full report →

Validate the orderByExpression field using EXPLAIN ESTIMATE queries
against ClickHouse, matching the existing HighlightedAttributeRow
validation pattern. Shows inline valid/invalid feedback with
debounced auto-validation and a manual validate button.

Made-with: Cursor
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

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


PR Review

No critical issues found.

The implementation is clean and well-structured. A few observations:

  • ℹ️ OrderByFormRow already includes useExplainQuery validation with a manual "Validate" button — the previous review concern about missing validation is addressed.
  • ℹ️ orderByExpression lives in SourceBaseSchema (all source kinds), but the OrderByFormRow UI is only rendered for Log and Trace. If a Metric/Session source somehow sets this field via API, getOrderBy in DBSearchPage.tsx:793 will silently use it. This appears intentional but is worth a brief comment in code.
  • ℹ️ The validate button (IconCheck) is only triggered manually — no auto-validation on blur/save. This is fine UX-wise since the field is optional, just noting it's different from other validated fields in the form.

The custom ORDER BY is applied as a silent default fallback in the
chart config, not written into the CodeMirror editor. Changed the
test to verify persistence via GET /sources and that search results
load successfully with the custom ORDER BY.

Made-with: Cursor
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

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


PR Review

  • Read changed files
  • Post review findings

No critical issues found.

The implementation is clean and well-structured across schema, model, UI, and query logic. The previous validation concern is addressed — OrderByFormRow uses useExplainQuery with a manual "Validate" button. Tests cover the key behaviors (custom expression, empty string fallback, undefined fallback).

One minor note: orderByExpression lives in SourceBaseSchema (all source kinds) but OrderByFormRow is only rendered for Log/Trace forms. If a Metric/Session source ever has this field set via API, useDefaultOrderBy in DBSearchPage.tsx:793 would silently use it. This is likely intentional given the current scope, but worth a brief code comment.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Claude finished @dhable's task in 29s —— View job


PR Review

No critical issues found.

The implementation is clean and well-structured. The previous validation concern is fully addressed — OrderByFormRow uses useExplainQuery with debounced auto-validation and a manual trigger button.

  • ℹ️ OrderByFormRow is rendered twice in SourceForm.tsx — once inside a <> fragment (for Log) and once inline (for Trace). The diff shows two separate <OrderByFormRow .../> call sites. This is correct and intentional, just worth confirming both render paths pass the right control prop (they do, based on the diff).
  • ℹ️ orderByExpression lives in SourceBaseSchema (all source kinds), but UI is only shown for Log/Trace. If a Metric/Session source ever has this field set via API, useDefaultOrderBy at DBSearchPage.tsx:794 will silently use it. Intentional given current scope, but worth a brief comment.
  • ℹ️ Validation fires on useDidUpdate (skips initial mount), so a pre-populated orderByExpression won't auto-validate until the user edits it — they must click the validate button. Minor UX note, not a bug.

tableName: z.string().min(1, 'Table is required'),
}),
querySettings: QuerySettingsSchema.optional(),
orderByExpression: z.string().optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be added to LogSourceAugmentation and TraceSourceAugmentation instead, since it is not supported on Session and Metric sources?

Comment on lines +949 to +953
<InputControlled
control={control}
name="orderByExpression"
placeholder="e.g. Timestamp DESC"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use SQLInlineEditorControlled here since the input is sql, for consistent styling / syntax highlighting with other SQL inputs, and for auto complete?

Comment on lines +969 to +981
{shouldShowResult && (
<Box>
{isExpressionValid && (
<Text c="green" size="xs">
Expression is valid.
</Text>
)}
{isExpressionInvalid && (
<ErrorCollapse
summary="Expression is invalid"
details={explainError?.message}
/>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - the validation result is all the way to the left here despite being aligned with the input on the highlighted expression fields

Image

Comment on lines +944 to +945
label="Order By"
helpText="Custom ORDER BY expression that overrides the default ordering. Leave empty to use the auto-detected default."
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit - for consistency with Default Select

Image
Suggested change
label="Order By"
helpText="Custom ORDER BY expression that overrides the default ordering. Leave empty to use the auto-detected default."
label="Default Order By"
helpText="Custom ORDER BY expression that overrides the default ordering. Leave empty to use the auto-detected default. (This can be customized per search later)"

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