feat(webui): Display timeline for guided Presto search.#1376
Conversation
WalkthroughIntroduces a guided Presto SQL flow: adds query builders and submit/cancel handlers, integrates timeline zoom-driven requerying, refactors timeline rendering to engine-specific components, updates timestamp handling to Dayjs/unix in SQL builder, and gates timeline visibility based on Presto SQL interface. Minor whitespace tweak in a Presto request file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RunBtn as GuidedRunButton
participant Store as Search Store
participant Builder as buildPrestoGuidedQueries
participant Guided as handlePrestoGuidedQuerySubmit
participant Backend as Query Service
User->>RunBtn: Click
RunBtn->>Store: Read timeRangeOption/timeRange
alt Non-CUSTOM time range
RunBtn->>Store: updateTimeRange(resolved via map)
end
RunBtn->>Store: updateTimelineConfig(compute from range)
RunBtn->>Builder: build searchQueryString + timelineQueryString
Builder-->>RunBtn: {search, timeline}
RunBtn->>Guided: submit({search, timeline})
Guided->>Store: reset UI, mark pending
par Submit search
Guided->>Backend: POST search query
Backend-->>Guided: searchJobId
and Submit timeline
Guided->>Backend: POST aggregation query
Backend-->>Guided: aggregationJobId
end
Guided->>Store: save jobIds, set UI state
sequenceDiagram
autonumber
participant TL as PrestoResultsTimeline
participant Store as Search Store
participant Builder as buildPrestoGuidedQueries
participant Guided as handlePrestoGuidedQuerySubmit
participant Backend as Query Service
TL->>TL: User zooms timeline
TL->>Store: updateTimelineConfig(new)
TL->>Store: updateTimeRange(new), set timeRangeOption=CUSTOM
TL->>Builder: build queries for new range
Builder-->>TL: {search, timeline}
TL->>Guided: submit({search, timeline})
Guided->>Backend: run search + aggregation
Backend-->>Guided: job ids
Guided->>Store: update job ids/UI state
sequenceDiagram
autonumber
actor User
participant Cancel as CancelButton
participant Guided as handlePrestoGuidedQueryCancel
participant Legacy as handlePrestoQueryCancel
participant Store as Search Store
User->>Cancel: Click
Cancel->>Store: Read sqlInterface, jobIds
alt Presto GUIDED and has aggregationJobId
Cancel->>Guided: cancel(searchJobId, aggregationJobId)
Guided->>Store: set UI DONE
else Non-guided or no aggregationJobId
Cancel->>Legacy: cancel(searchJobId)
Legacy->>Store: set UI DONE
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
davemarco
left a comment
There was a problem hiding this comment.
Overall looks good. I have clean up comments
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
components/webui/client/src/sql-parser/index.ts (1)
180-181: Critical: Wrong JOIN direction drops zero-count buckets.The query uses
FROM buckets LEFT JOIN timestamps, which only returns rows where buckets exist. This drops empty time intervals (zero counts) from the timeline, making it incomplete.For a complete timeline showing all time intervals, drive from
timestampsand left-joinbuckets:SELECT COALESCE(cnt, 0) AS count, CAST(timestamp AS double) AS timestamp -FROM buckets -LEFT JOIN timestamps ON buckets.idx = timestamps.idx +FROM timestamps +LEFT JOIN buckets ON buckets.idx = timestamps.idx ORDER BY timestamps.idxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (1)
88-94: Fix disabled predicate and surface errors without rethrowing
!isQueryReadyviolates the “prefer `false === …`” guideline, and rethrowing insideonClickrecreates the unhandled rejection we had before. Please adopt the earlier suggestion: compare explicitly againstfalseand handle the rejection locally (log or surface a toast) instead of rethrowing.- disabled={!isQueryReady || + disabled={false === isQueryReady || searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} - onClick={() => { - handleClick().catch((err: unknown) => { - throw err; - }); - }} + onClick={async () => { + try { + await handleClick(); + } catch (err) { + console.error(err); + } + }}As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx(4 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(2 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/PrestoVirtualResultsTimeline.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/VirtualResultsTimeline.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/index.tsx(2 hunks)components/webui/client/src/sql-parser/index.ts(4 hunks)components/webui/client/src/sql-parser/typings.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsxcomponents/webui/client/src/sql-parser/typings.tscomponents/webui/client/src/sql-parser/index.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/VirtualResultsTimeline.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/PrestoVirtualResultsTimeline.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx
🧬 Code graph analysis (8)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
handlePrestoQueryCancel(111-111)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (1)
handlePrestoGuidedQueryCancel(197-197)
components/webui/client/src/sql-parser/typings.ts (1)
components/webui/client/src/sql-parser/index.ts (2)
BuildSearchQueryProps(202-202)BuildTimelineQueryProps(203-203)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/VirtualResultsTimeline.tsx (7)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts (1)
useAggregationResults(49-49)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/useAggregationResults.ts (1)
useAggregationResults(36-36)components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (1)
TIME_RANGE_OPTION(130-130)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)components/webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(26-26)SETTINGS_STORAGE_ENGINE(28-28)components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (1)
handleQuerySubmit(148-148)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts (3)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/useAggregationResults.ts (1)
useAggregationResults(36-36)components/webui/client/src/api/socket/useCursor.tsx (1)
useCursor(78-78)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (3)
components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)components/webui/client/src/sql-parser/index.ts (2)
buildSearchQuery(195-195)buildTimelineQuery(196-196)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/index.tsx (1)
components/webui/client/src/config/index.ts (1)
SETTINGS_QUERY_ENGINE(27-27)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/PrestoVirtualResultsTimeline.tsx (5)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts (1)
useAggregationResults(49-49)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/useAggregationResults.ts (1)
useAggregationResults(36-36)components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (1)
TIME_RANGE_OPTION(130-130)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (2)
buildPrestoGuidedQueries(196-196)handlePrestoGuidedQuerySubmit(198-198)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/GuidedRunButton.tsx (2)
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (2)
TIME_RANGE_OPTION(130-130)TIME_RANGE_OPTION_DAYJS_MAP(131-131)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-guided-search-requests.ts (2)
buildPrestoGuidedQueries(196-196)handlePrestoGuidedQuerySubmit(198-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: package-image
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (3)
components/webui/client/src/sql-parser/typings.ts (1)
1-1: LGTM! Type safety improvement.The change from
numbertoDayjsfor timestamp properties provides better type safety and aligns with the PR's objective to standardize time handling using Dayjs across the guided Presto search functionality.Also applies to: 10-11, 17-18
components/webui/client/src/sql-parser/index.ts (2)
107-107: LGTM! Correct use of Unix seconds.The use of
.unix()correctly converts Dayjs timestamps to Unix seconds (seconds since epoch) for SQL operations withto_unixtime(), ensuring proper timestamp comparisons in the WHERE clause and width_bucket bounds.Also applies to: 163-164, 168-168
149-153: Unit inconsistency: timestamps in milliseconds vs width_bucket in seconds.The
timestampsarray contains milliseconds (fromvalueOf()), butwidth_bucketoperates on Unix seconds. While the join is based on bucket indices (not timestamp values), this unit mismatch creates confusion and could lead to display issues if consumers expect timestamps aligned with the bucket boundaries.Consider changing line 152 to generate Unix seconds:
- (_, i) => Math.floor(startTimestamp.valueOf() + (i * step)) + (_, i) => Math.floor((startTimestamp.valueOf() + (i * step)) / 1000)Or adjust the step calculation to work in seconds from the start:
- const step = (endTimestamp.valueOf() - startTimestamp.valueOf()) / bucketCount; + const step = (endTimestamp.unix() - startTimestamp.unix()) / bucketCount; const timestamps = Array.from( {length: bucketCount}, - (_, i) => Math.floor(startTimestamp.valueOf() + (i * step)) + (_, i) => Math.floor(startTimestamp.unix() + (i * step)) );Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx (1)
34-34: Address the previous review feedback.The condition on Line 34 still has not addressed the feedback from the previous review:
- It uses a hard-coded
nullinstead of theSEARCH_STATE_DEFAULT.aggregationJobIdsentinel- It uses
===instead of==for the boolean check, which does not follow the coding guidelineAs suggested in the previous review, please apply this diff:
-import useSearchStore from "../../../../SearchState/index"; +import useSearchStore, {SEARCH_STATE_DEFAULT} from "../../../../SearchState/index"; ... - if (false === isPrestoGuided || null === aggregationJobId) { + if ( + false == isPrestoGuided || + SEARCH_STATE_DEFAULT.aggregationJobId === aggregationJobId + ) { handlePrestoQueryCancel({searchJobId}); } else { handlePrestoGuidedQueryCancel(searchJobId, aggregationJobId); }Based on coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts (2)
components/webui/client/src/api/socket/useCursor.tsx (1)
useCursor(78-78)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
🔇 Additional comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx (2)
10-12: LGTM!The new imports are correctly added to support the guided Presto cancellation flow.
24-26: LGTM!The state access and derived flag are implemented correctly.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/Presto/useAggregationResults.ts (2)
15-15: LGTM! Selector pattern correctly applied.The aggregation job ID is now selected directly from the store using a selector function, which prevents unnecessary re-renders when other search state fields change. This addresses the past review feedback.
35-44: LGTM! Transformation logic is clear and correct.The memoized transformation correctly unwraps the aggregation results from
{row: TimelineBucket}[]toTimelineBucket[], matching the expected format described in the PR objectives. The null check and array mapping are both appropriate.
| return null; | ||
| } | ||
|
|
||
| const cursor = aggregationResultsCursor as {row: TimelineBucket}[]; |
There was a problem hiding this comment.
return cursor?.map(({ row }) => row) ?? null;
Is this simpler? I feel like this will only update when the data changes, so the memo isnt helping that much
There was a problem hiding this comment.
if there is some annoying thing with doing this in a hook, u can probably just take this logic outside the hook
There was a problem hiding this comment.
so u disagree here i guess?
There was a problem hiding this comment.
I think I've already made the change?
I agree with you. We only need return cursor?.map(({ row }) => row) ?? null; if not multiplexing the hook. I didn't clean it up when separating the two hooks.
It looks that we cannot put the logic outside hook. It confuses react and causes infinite recursive rendering?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * | ||
| * @return | ||
| */ | ||
| const ClpResultsTimeline = () => { |
There was a problem hiding this comment.
Both resultstimeline and searchresultstimeline are already used. How about using ClpResultsTimeline? @davemarco
There was a problem hiding this comment.
sorry last change.. I was using Native (for clp and clp-s) for this. Like NativeResultsTimeline
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| * | ||
| * @return | ||
| */ | ||
| const ClpResultsTimeline = () => { |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @davemarco 's review
Description
This PR shows the timeline display for guided Presto search.
Changes:
{row: TimelineBucket}[]toTimelineBucket[]inuseAggregationResultChecklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Refactor
Style