feat(webui): Add support for submitting Presto SQL queries. #1127
Conversation
WalkthroughThis change introduces backend and frontend support for submitting Presto SQL queries. It adds a Fastify plugin for Presto integration, API routes and schemas for query submission, client-side request handlers, and updates the Presto RunButton to trigger query submission. Configuration and dependency updates for Presto connectivity are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RunButton (UI)
participant Client Handler
participant API Server
participant Presto Plugin
participant Presto Server
User->>RunButton (UI): Click "Run"
RunButton (UI)->>Client Handler: handlePrestoQuerySubmit(queryString)
Client Handler->>API Server: POST /api/presto-search/query {queryString}
API Server->>Presto Plugin: Execute queryString via Presto client
Presto Plugin->>Presto Server: Submit query
Presto Server-->>Presto Plugin: Respond with job ID/state
Presto Plugin-->>API Server: Return searchJobId
API Server-->>Client Handler: {searchJobId}
Client Handler-->>RunButton (UI): Log/notify job creation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: in `components/log-viewer-webui/client/src/api/query.js`, the `extractjsonresp` type definition is a...Applied to files:
📚 Learning: user haiqi96 requested creating a github issue to document a bug fix from pr #1136, which addressed ...Applied to files:
📚 Learning: user junhaoliao requested creating a github issue to track server-side sql query error handling impr...Applied to files:
📚 Learning: in components/log-viewer-webui/server/src/routes/static.ts, when decoratereply is set to true in fas...Applied to files:
📚 Learning: the new webui architecture uses fastify with pino logging instead of the previous winston-based logg...Applied to files:
📚 Learning: do not suggest modifications to auto-generated code files, even for minor issues like duplicate decl...Applied to files:
📚 Learning: when implementing socket.io namespaces or connection pooling in a client-server architecture (like t...Applied to files:
📚 Learning: for internal apis in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of pa...Applied to files:
🧬 Code Graph Analysis (1)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (2)
⏰ 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). (1)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hoophalab
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/webui/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
components/webui/client/src/api/presto-search.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx(3 hunks)components/webui/server/package.json(2 hunks)components/webui/server/settings.json(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)components/webui/server/src/schemas/presto-search.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/RunButton/index.tsxcomponents/webui/server/src/schemas/presto-search.tscomponents/webui/client/src/api/presto-search.tscomponents/webui/server/src/routes/api/presto-search/index.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/package.json (2)
Learnt from: junhaoliao
PR: #939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: #596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In components/log-viewer-webui/client/src/api/query.js, the ExtractJsonResp type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/schemas/presto-search.ts (1)
Learnt from: junhaoliao
PR: #596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In components/log-viewer-webui/client/src/api/query.js, the ExtractJsonResp type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/client/src/api/presto-search.ts (1)
Learnt from: junhaoliao
PR: #596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In components/log-viewer-webui/client/src/api/query.js, the ExtractJsonResp type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/routes/api/presto-search/index.ts (2)
Learnt from: junhaoliao
PR: #596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In components/log-viewer-webui/client/src/api/query.js, the ExtractJsonResp type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: #939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
🔇 Additional comments (6)
components/webui/server/src/schemas/presto-search.ts (1)
1-21: LGTM! Well-structured schema definitions.The schema definitions are clean, well-organized, and follow TypeBox conventions. The separation of concerns between job creation and job response schemas is appropriate.
components/webui/server/package.json (1)
41-41: Verified Presto Client dependency versions and security
- components/webui/server/package.json (lines 41 & 46):
- presto-client@^1.1.0 and @types/presto-client@^1.0.2 are the latest stable releases.
- No known security vulnerabilities have been reported for either package.
- Note: presto-client was last updated Jan 2024 and @types/presto-client Sep 2024; it’s advisable to monitor for future updates.
No further changes required.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx (1)
26-35: LGTM! Good implementation with proper async handling.The click handler is well-implemented with proper memoization, error handling, and state management. The use of
useCallbackwith the correct dependency array prevents unnecessary re-renders.components/webui/client/src/api/presto-search.ts (1)
38-41: LGTM! Good API design with proper validation.The
submitQueryfunction is well-implemented with proper TypeScript typing and response validation using TypeBox. The use ofValue.Parseensures runtime type safety.components/webui/server/src/routes/api/presto-search/index.ts (2)
1-12: Imports are well-organized and appropriate.The imports are properly structured with external dependencies first, followed by local imports. The JSON import assertion for settings is correctly implemented.
78-85: Consider the implications of returning immediately after receiving the job ID.The current implementation returns the search job ID as soon as the state callback is invoked, without waiting for the query to complete. This might be intentional for async job submission, but it's worth documenting this behavior clearly.
If this is the intended behavior, consider adding a comment to clarify:
state: (error, newSearchJobId, stats) => { request.log.info({ error: error, searchJobId: newSearchJobId, state: stats.state, }, "Presto search state updated"); + // Return the job ID immediately for async tracking resolve(newSearchJobId); },
components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx
Outdated
Show resolved
Hide resolved
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | ||
| if (false === settings.EnablePresto) { | ||
| return; | ||
| } | ||
|
|
||
| const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort}); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for Presto configuration settings.
The code should validate that PrestoHost and PrestoPort are properly configured before creating the client. Consider adding checks to ensure these settings exist and are valid.
const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
if (false === settings.EnablePresto) {
return;
}
+ if (!settings.PrestoHost || !settings.PrestoPort) {
+ throw new Error("Presto is enabled but PrestoHost or PrestoPort is not configured");
+ }
+
const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | |
| if (false === settings.EnablePresto) { | |
| return; | |
| } | |
| const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort}); | |
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | |
| if (false === settings.EnablePresto) { | |
| return; | |
| } | |
| if (!settings.PrestoHost || !settings.PrestoPort) { | |
| throw new Error("Presto is enabled but PrestoHost or PrestoPort is not configured"); | |
| } | |
| const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort}); | |
| // …rest of plugin implementation… | |
| } |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/presto-search/index.ts around lines 19
to 25, the code creates a Presto client without validating the configuration
settings. Add validation checks to ensure that settings.PrestoHost is a
non-empty string and settings.PrestoPort is a valid number before creating the
client. If either setting is missing or invalid, log an error or return early to
prevent creating the client with invalid configuration.
| searchJobId = await new Promise((resolve) => { | ||
| client.execute({ | ||
| data: (error, data, columns) => { | ||
| if (null === searchJobId) { | ||
| request.log.error("Data arrived from Presto before the search job id."); | ||
| } | ||
| request.log.info({error, searchJobId, columns, data}, "Presto data"); | ||
| }, | ||
| error: (error) => { | ||
| request.log.info(error, "Presto search failed"); | ||
|
|
||
| // The `/query` endpoint will catch this error if `resolve` hasn't | ||
| // been called. | ||
| throw new Error("Presto search failed."); | ||
| }, | ||
| query: queryString, | ||
| state: (error, newSearchJobId, stats) => { | ||
| request.log.info({ | ||
| error: error, | ||
| searchJobId: newSearchJobId, | ||
| state: stats.state, | ||
| }, "Presto search state updated"); | ||
|
|
||
| resolve(newSearchJobId); | ||
| }, | ||
| success: () => { | ||
| request.log.info("Presto search succeeded"); | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Improve error handling and async flow in Presto callbacks.
The current implementation has several issues:
- The error thrown in the error callback (line 60) won't be caught by the Promise
- The Promise resolves immediately when state is updated, not when the query completes
- No handling for the case where the query completes without calling the state callback
Consider refactoring to properly handle all callback scenarios:
- searchJobId = await new Promise((resolve) => {
+ searchJobId = await new Promise((resolve, reject) => {
client.execute({
data: (error, data, columns) => {
+ if (error) {
+ request.log.error({error}, "Error in data callback");
+ return;
+ }
if (null === searchJobId) {
request.log.error("Data arrived from Presto before the search job id.");
}
request.log.info({error, searchJobId, columns, data}, "Presto data");
},
error: (error) => {
request.log.info(error, "Presto search failed");
-
- // The `/query` endpoint will catch this error if `resolve` hasn't
- // been called.
- throw new Error("Presto search failed.");
+ reject(new Error("Presto search failed."));
},
query: queryString,
state: (error, newSearchJobId, stats) => {
+ if (error) {
+ request.log.error({error}, "Error in state callback");
+ reject(error);
+ return;
+ }
request.log.info({
error: error,
searchJobId: newSearchJobId,
state: stats.state,
}, "Presto search state updated");
- resolve(newSearchJobId);
+ // Only set searchJobId once
+ if (null === searchJobId && newSearchJobId) {
+ searchJobId = newSearchJobId;
+ }
+
+ // Don't resolve here - wait for success or error
},
success: () => {
request.log.info("Presto search succeeded");
+ resolve(searchJobId);
},
});
});🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/presto-search/index.ts between lines
47 and 76, the Promise handling Presto client callbacks has issues: throwing an
error inside the error callback does not reject the Promise, the Promise
resolves prematurely on state update instead of query completion, and there is
no handling for query completion without state callback. To fix this, refactor
the Promise to reject on error by calling reject with the error, resolve only
when the success callback is invoked indicating query completion, and ensure all
possible callback scenarios (error, state, success) properly trigger resolve or
reject to maintain correct async flow.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
(i haven't validated the changes) on a high level, the code lgtm
we shall see if "@prestodb/presto-js-client" can be a better choice than "presto-client" before making further changes
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
components/webui/server/settings.json (1)
21-23: Address past review feedback and port conflict concern.The configuration still uses hardcoded localhost values and port 8080, which conflicts with the commonly used docs debug server port as noted in previous reviews. The naming also differs from the suggested interface using
ClpQueryEngineinstead of the previously suggested approach.Consider the following improvements:
- Use environment variables for flexible deployment
- Change the default port to avoid conflicts (e.g., 8081)
- Add authentication settings for production security
- "ClpQueryEngine": "presto", - "PrestoHost": "localhost", - "PrestoPort": 8080 + "ClpQueryEngine": "${CLP_QUERY_ENGINE:-native}", + "PrestoHost": "${PRESTO_HOST:-localhost}", + "PrestoPort": "${PRESTO_PORT:-8081}"components/webui/server/src/routes/api/presto-search/index.ts (2)
44-78: Critical: Fix Promise handling and async flow issues.The current Promise implementation has several critical issues that were highlighted in previous reviews:
- The Promise resolves immediately when state is updated, not when the query completes
- Error handling in callbacks doesn't properly reject the Promise
- No handling for successful query completion
- searchJobId = await new Promise<string>((resolve, reject) => { - let isResolved = false; + searchJobId = await new Promise<string>((resolve, reject) => { + let queryId: string | null = null; + let isResolved = false; Presto.client.execute({ - // eslint-disable-next-line no-warning-comments - // TODO: Data, error, and success handlers are dummy implementations - // and should be completed. data: (_, data, columns) => { + if (null === queryId) { + request.log.error("Data arrived from Presto before the search job id."); + } request.log.info({columns, data}, "Presto data"); }, error: (error) => { request.log.info(error, "Presto search failed"); if (false === isResolved) { isResolved = true; - reject(new Error("Presto search failed")); + reject(new Error("Presto search failed")); } }, query: queryString, state: (_, newQueryId, stats) => { request.log.info({ - searchJobId: queryId, + searchJobId: newQueryId, state: stats.state, }, "Presto search state updated"); - if (false === isResolved) { - isResolved = true; - resolve(queryId); - } + // Store query ID but don't resolve yet + if (null === queryId && newQueryId) { + queryId = newQueryId; + } }, success: () => { request.log.info("Presto search succeeded"); + if (false === isResolved) { + isResolved = true; + if (queryId) { + resolve(queryId); + } else { + reject(new Error("Query succeeded but no query ID received")); + } + } }, }); });
16-21: Improve error message for missing Presto client.The error message could be more descriptive to help with debugging.
- if ("undefined" === typeof Presto) { - throw new Error("Presto not available"); - } + if ("undefined" === typeof Presto) { + throw new Error("Presto client is not available. Ensure ClpQueryEngine is set to 'presto' and Presto configuration is valid."); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/webui/client/public/settings.json(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(1 hunks)components/webui/server/settings.json(1 hunks)components/webui/server/src/plugins/app/Presto.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)components/webui/server/src/schemas/presto-search.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/RunButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/server/src/schemas/presto-search.tscomponents/webui/server/src/routes/api/presto-search/index.tscomponents/webui/server/src/plugins/app/Presto.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
📚 Learning: in `components/log-viewer-webui/client/src/api/query.js`, the `extractjsonresp` type definition is a...
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Applied to files:
components/webui/client/public/settings.jsoncomponents/webui/server/settings.jsoncomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/server/src/schemas/presto-search.tscomponents/webui/server/src/routes/api/presto-search/index.ts
📚 Learning: in the clp codebase, the `m_case_sensitive_search` flag is used only for actual string value compari...
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:320-324
Timestamp: 2025-05-05T01:12:18.561Z
Learning: In the CLP codebase, the `m_case_sensitive_search` flag is used only for actual string value comparisons during query evaluation, not for schema key name matching. Schema keys are always compared case-sensitively regardless of this flag's setting.
Applied to files:
components/webui/client/public/settings.json
📚 Learning: in the clp codebase, `table_prefix` is a trusted configuration value and can be safely embedded into...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:90-96
Timestamp: 2024-11-15T16:22:33.635Z
Learning: In the CLP codebase, `table_prefix` is a trusted configuration value and can be safely embedded into SQL queries.
Applied to files:
components/webui/client/public/settings.json
📚 Learning: user haiqi96 requested creating a github issue to document a bug fix from pr #1136, which addressed ...
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
Applied to files:
components/webui/server/settings.json
📚 Learning: user junhaoliao requested creating a github issue to track server-side sql query error handling impr...
Learnt from: junhaoliao
PR: y-scope/clp#1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsxcomponents/webui/server/src/routes/api/presto-search/index.ts
📚 Learning: for internal apis in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of pa...
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx
📚 Learning: in the y-scope/clp react webui client codebase, for zustand store usage: use `usestore.getstate().me...
Learnt from: hoophalab
PR: y-scope/clp#1108
File: components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx:15-15
Timestamp: 2025-07-18T20:00:50.288Z
Learning: In the y-scope/clp React webui client codebase, for Zustand store usage: use `useStore.getState().method` for callbacks since the output is not reactive and doesn't need state as a dependency in the hook, and use `useStore((state) => state.property)` with proper selectors for reactive components that need to re-render when state changes.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx
📚 Learning: the new webui architecture uses fastify with pino logging instead of the previous winston-based logg...
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Applied to files:
components/webui/server/src/routes/api/presto-search/index.tscomponents/webui/server/src/plugins/app/Presto.ts
📚 Learning: in components/log-viewer-webui/server/src/routes/static.ts, when decoratereply is set to true in fas...
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Applied to files:
components/webui/server/src/routes/api/presto-search/index.ts
📚 Learning: do not suggest modifications to auto-generated code files, even for minor issues like duplicate decl...
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/kql/antlr_generated/KqlLexer.cpp:8-13
Timestamp: 2025-05-26T18:41:26.096Z
Learning: Do not suggest modifications to auto-generated code files, even for minor issues like duplicate declarations or formatting inconsistencies.
Applied to files:
components/webui/server/src/routes/api/presto-search/index.ts
🪛 GitHub Actions: clp-lint
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
[error] 1-26: ESLint errors: Unable to resolve path to module '../../../../api/presto-search' (import/no-unresolved); duplicated import '../../../../api/presto-search' (no-duplicate-imports); expected 2 empty lines after import statement (import/newline-after-import); unsafe calls and member accesses on 'error' typed values (@typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access); unsafe assignment of 'any' value (@typescript-eslint/no-unsafe-assignment); unexpected line breaks in object curly braces (@stylistic/object-curly-newline). 12 errors total.
[warning] 1-1: ESLint warning: Run autofix to sort these imports! (simple-import-sort/imports)
🔇 Additional comments (12)
components/webui/client/public/settings.json (1)
3-3: LGTM! Configuration aligns with server-side changes.The change to set
ClpQueryEngineto "presto" correctly aligns the client configuration with the server-side Presto integration.components/webui/client/src/pages/SearchPage/SearchControls/Presto/RunButton/index.tsx (4)
1-1: LGTM! Clean import addition.The
useCallbackimport is correctly added for the memoized click handler.
10-10: LGTM! Proper import of the query submission handler.The import of
handlePrestoQuerySubmitcorrectly integrates the button with the Presto query submission flow.
26-28: LGTM! Proper memoization with correct dependencies.The
useCallbackhook correctly memoizes the click handler withqueryStringas a dependency, preventing unnecessary re-renders while ensuring the handler uses the current query string.
38-38: LGTM! onClick handler properly wired.The button's
onClickevent is correctly connected to the memoizedhandleClickfunction.Note: The error handling concern from previous reviews about user-friendly error feedback still applies to the
handlePrestoQuerySubmitfunction in the imported module.components/webui/server/src/schemas/presto-search.ts (1)
1-23: LGTM! Well-structured schema definitions.The schema definitions are clean and well-organized:
- Proper use of TypeBox for runtime validation
- Clear separation between creation and identification schemas
- Good reuse of common StringSchema
- Comprehensive documentation
components/webui/server/src/plugins/app/Presto.ts (3)
1-8: LGTM! Clean imports and configuration loading.The import structure is well-organized, correctly importing the necessary Presto client types and using the new JSON import syntax with type assertion.
13-22: Simple and effective Presto client wrapper.The Presto class provides a clean abstraction around the Presto client with proper type safety and readonly access to the client instance.
24-28: Proper Fastify module augmentation.The module declaration correctly extends the FastifyInstance interface to include the optional Presto property, following Fastify plugin patterns.
components/webui/server/src/routes/api/presto-search/index.ts (3)
1-8: LGTM! Proper imports and schema usage.The imports are well-structured, correctly bringing in Fastify types, HTTP status codes, and the necessary schemas for validation.
26-37: LGTM! Well-defined route schema.The route configuration properly defines the schema with appropriate request/response validation and OpenAPI tags.
84-87: LGTM! Proper response handling.The response correctly sets the HTTP status code and returns the expected payload structure.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Outdated
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Show resolved
Hide resolved
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Data, error, and success handlers are dummy implementations | ||
| // and should be completed. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove outdated TODO comment.
The TODO comment about dummy implementations should be removed since the handlers are now properly implemented.
- // eslint-disable-next-line no-warning-comments
- // TODO: Data, error, and success handlers are dummy implementations
- // and should be completed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line no-warning-comments | |
| // TODO: Data, error, and success handlers are dummy implementations | |
| // and should be completed. | |
| request.log.info({ | |
| error: error, | |
| searchJobId: newSearchJobId, | |
| state: stats.state, | |
| }, "Presto search state updated"); | |
| success: () => { | |
| request.log.info("Presto search succeeded"); | |
| resolve(searchJobId); | |
| }, |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/presto-search/index.ts around lines 49
to 51, remove the outdated TODO comment that states the data, error, and success
handlers are dummy implementations, as these handlers have now been properly
implemented and the comment is no longer relevant.
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
I did some refactoring such as
- creating a seperate plugin for presto
- moved the query submit logic into presto-search-requests.ts to be more similiar to native ui
- Restructured route code to have better error handling
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
didn't test but the code lgtm. @hoophalab please do a final quick look
There is a kill api no? Like the client just sends back the id, and we kill the query |
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
lgtm
Validations:
-
Executed
SELECT * FROM tpch.sf100.customer limit 10in the Presto search webui and verified that the data appeared correctly in the server's console. -
Executed
SELECT * FROM tpch.sf100.customerand verified that the data streamed as intended to the server's console.
I was thinking when the user sends an ID to cancel a query, we need to tell This means we'll need a map to link search job IDs to their cancellation status. But storing state in server's memory might not be ideal for failure recovery or out-of-memory issues. However, we should be fine with merely storing ephemeral search job ids. So, let's proceed with that approach. |
|
@hoophalab - so your saying we cant just use this https://github.com/tagomoris/presto-client-node?tab=readme-ov-file#killquery_id-callback? |
I didn't notice this function before. This function looks correct and we can use it. |
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @hoophalab and @junhaoliao's reviews.
hoophalab
dismissed
junhaoliao’s stale review
changes have already been applied
| "ClpQueryEngine": "native", | ||
| "PrestoHost": "localhost", | ||
| "PrestoPort": 8889 |
There was a problem hiding this comment.
we should also configure these in server_settings_json_updates in components/clp-package-utils/clp_package_utils/scripts/start_clp.py
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
I'll examine the current structure of the start_clp.py file and then create an issue to track this configuration work.
🏁 Script executed:
#!/bin/bash
# Examine the start_clp.py file to understand the server_settings_json_updates structure
cat components/clp-package-utils/clp_package_utils/scripts/start_clp.py | grep -A 20 -B 5 "server_settings_json_updates"
Length of output: 2869
Now I'll create a GitHub issue to track this configuration work.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the Presto settings configuration in start_clp.py
gh issue create \
--title "Configure Presto settings in start_clp.py server_settings_json_updates" \
--body "## Description
The recent addition of Presto SQL query support in PR #1127 introduced new configuration settings in \`components/webui/server/settings.json\`:
- \`ClpQueryEngine\`: \"native\"
- \`PrestoHost\`: \"localhost\"
- \`PrestoPort\`: 8889
These settings need to be properly configured in the \`server_settings_json_updates\` dictionary in \`components/clp-package-utils/clp_package_utils/scripts/start_clp.py\` to ensure consistency between the webui server configuration and the CLP startup script.
## Current Status
The settings are currently hardcoded in \`settings.json\` but are not dynamically configured through the startup script's configuration management system.
## Expected Behavior
The Presto-related settings should be configurable through the CLP configuration system and properly injected into the webui server settings during startup, similar to how other settings like database and MongoDB configurations are handled.
## References
- PR: https://github.com/y-scope/clp/pull/1127
- Comment: https://github.com/y-scope/clp/pull/1127#discussion_r2296622851
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 1275
@junhaoliao I've created issue #1260 to track the configuration of the new Presto settings in start_clp.py.
The issue documents the need to add the three new settings (ClpQueryEngine, PrestoHost, and PrestoPort) to the server_settings_json_updates dictionary so they can be properly configured through the CLP configuration system rather than being hardcoded in settings.json.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.
4 participants
Description
@davemarco, after experimenting with presto-client-node, I feel we'll still need a
PrestoSearchManagerfor handling cancel requests.Also, we are probably going to store presto query's states in server's memory, which doesn't sound good.
Checklist
breaking change.
Validation performed
Executed
SELECT * FROM tpch.sf100.customer limit 10in the Presto search webui and verified that the data appeared correctly in the server's console.Executed
SELECT * FROM tpch.sf100.customerand verified that the data streamed as intended to the server's console.Summary by CodeRabbit
New Features
Chores