-
Notifications
You must be signed in to change notification settings - Fork 82
feat(webui): Add support for cancelling Presto SQL queries. #1175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
28dd131
d125a51
3d2f89e
5cbdce7
aa419fa
3096ecf
410bd5b
4171efa
acafd2e
b7a4a01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.cancelButton { | ||
width: 100%; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,47 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {useCallback} from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {CloseOutlined} from "@ant-design/icons"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Button, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Tooltip, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from "antd"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import useSearchStore from "../../../../SearchState/index"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {handlePrestoQueryCancel} from "../../presto-search-requests"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import styles from "./index.module.css"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Renders a button to cancel the SQL query. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const CancelButton = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const searchJobId = useSearchStore((state) => state.searchJobId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const handleClick = useCallback(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (null === searchJobId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
console.error("Cannot cancel query: searchJobId is not set."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
handlePrestoQueryCancel({searchJobId}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, [searchJobId]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Use getState() in event handler to avoid stale deps and re-creations. For non-reactive callbacks, prefer reading the latest store value at call-time. - const handleClick = useCallback(() => {
- if (null === searchJobId) {
+ const handleClick = useCallback(() => {
+ const {searchJobId} = useSearchStore.getState();
+ if (null === searchJobId) {
console.error("Cannot cancel query: searchJobId is not set.");
return;
}
handlePrestoQueryCancel({searchJobId});
- }, [searchJobId]);
+ }, []); 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<Tooltip title={"Cancel query"}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<Button | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className={styles["cancelButton"] || ""} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
color={"red"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
icon={<CloseOutlined/>} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
size={"large"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
variant={"solid"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onClick={handleClick} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Cancel | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</Button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify antd Button props Ant Design typically uses Run to confirm antd version and whether 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "antd version in package.json files:"
fd package.json | xargs -I{} jq -r '.dependencies.antd // .devDependencies.antd // empty' {} | nl -ba
echo
echo "Occurrences of Button with color/variant props in client code:"
rg --glob 'components/**/client/**' -n 'Button\s*\([\s\S]*?(color=|variant=)' -A 2 -B 2 Length of output: 336 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Looking for 'antd' in package.json files:"
rg '"antd"' -n -g 'package.json' || echo "No antd entry found in package.json"
echo
echo "Import of Button in CancelButton file:"
rg "import .*Button" -n components/webui/client/src/pages/SearchPage/SearchControls/Presto/PrestoSearchButton/CancelButton/index.tsx || echo "No Button import found"
echo
echo "Occurrences of <Button> with color prop:"
rg "<Button[^>]*color=" -n -g '*.tsx' || echo "None found in TSX files"
echo
echo "Occurrences of <Button> with variant prop:"
rg "<Button[^>]*variant=" -n -g '*.tsx' || echo "None found in TSX files" Length of output: 897 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Import statements in CancelButton/index.tsx:"
rg "^import" -n components/webui/client/src/pages/SearchPage/SearchControls/Presto/PrestoSearchButton/CancelButton/index.tsx Length of output: 523 Import Button and adjust props in CancelButton/index.tsx It looks like the • At the top of import { Button } from "antd"; (Or import your project’s custom Button wrapper if one exists.) Fixing these will restore correct typing and styling for the cancel button. 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</Tooltip> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Optional: Show cancelling progress rather than instant UI flip. Current flow flips to “Run” immediately (state set to DONE before the API returns). Consider keeping the Cancel button visible with a loading spinner until the cancel completes to avoid starting a new query while cancellation is in-flight. If you adopt the refactor in - <Button
+ <Button
className={styles["cancelButton"] || ""}
color={"red"}
icon={<CloseOutlined/>}
size={"large"}
variant={"solid"}
onClick={handleClick}
+ // Optionally: pass a prop like loading={searchUiState === SEARCH_UI_STATE.CANCEL_PENDING}
> 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default CancelButton; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.runButton { | ||
width: 100%; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,8 +6,10 @@ import { | |||||
Tooltip, | ||||||
} from "antd"; | ||||||
|
||||||
import useSearchStore from "../../../SearchState/index"; | ||||||
import {handlePrestoQuerySubmit} from "../presto-search-requests"; | ||||||
import useSearchStore from "../../../../SearchState/index"; | ||||||
import {SEARCH_UI_STATE} from "../../../../SearchState/typings"; | ||||||
import {handlePrestoQuerySubmit} from "../../presto-search-requests"; | ||||||
import styles from "./index.module.css"; | ||||||
|
||||||
|
||||||
/** | ||||||
|
@@ -16,6 +18,7 @@ import {handlePrestoQuerySubmit} from "../presto-search-requests"; | |||||
* @return | ||||||
*/ | ||||||
const RunButton = () => { | ||||||
const searchUiState = useSearchStore((state) => state.searchUiState); | ||||||
const queryString = useSearchStore((state) => state.queryString); | ||||||
|
||||||
const isQueryStringEmpty = "" === queryString; | ||||||
|
@@ -30,11 +33,13 @@ const RunButton = () => { | |||||
return ( | ||||||
<Tooltip title={tooltipTitle}> | ||||||
<Button | ||||||
className={styles["runButton"] || ""} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Avoid redundant className fallback. - className={styles["runButton"] || ""}
+ className={styles["runButton"]} 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||
color={"green"} | ||||||
disabled={isQueryStringEmpty} | ||||||
icon={<CaretRightOutlined/>} | ||||||
size={"large"} | ||||||
variant={"solid"} | ||||||
disabled={isQueryStringEmpty || | ||||||
searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING} | ||||||
onClick={handleClick} | ||||||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
> | ||||||
Run | ||||||
|
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.runButtonContainer { | ||
width: 120px; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Optional: avoid hard-coded width; consider a token or min-width A fixed 120px works but can be brittle on small screens. Consider a CSS variable (e.g., --search-button-width) or min-width/flex-basis to keep it responsive and consistent across themes. 🤖 Prompt for AI Agents
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import useSearchStore from "../../../SearchState/index"; | ||
import {SEARCH_UI_STATE} from "../../../SearchState/typings"; | ||
import CancelButton from "./CancelButton"; | ||
import styles from "./index.module.css"; | ||
import RunButton from "./RunButton"; | ||
|
||
|
||
/** | ||
* Renders a button to submit or cancel the SQL query. | ||
* | ||
* @return | ||
*/ | ||
const PrestoSearchButton = () => { | ||
const searchUiState = useSearchStore((state) => state.searchUiState); | ||
|
||
return ( | ||
<div className={styles["runButtonContainer"] || ""}> | ||
{ (searchUiState === SEARCH_UI_STATE.QUERYING) ? | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<CancelButton/> : | ||
<RunButton/>} | ||
</div> | ||
); | ||
}; | ||
|
||
export default PrestoSearchButton; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import { | ||
cancelQuery, | ||
type PrestoQueryJobCreationSchema, | ||
type PrestoQueryJobSchema, | ||
submitQuery, | ||
} from "../../../../api/presto-search"; | ||
import useSearchStore from "../../SearchState"; | ||
import {SEARCH_UI_STATE} from "../../SearchState/typings"; | ||
|
||
|
||
/** | ||
|
@@ -10,9 +14,30 @@ import { | |
* @param payload | ||
*/ | ||
const handlePrestoQuerySubmit = (payload: PrestoQueryJobCreationSchema) => { | ||
const {updateSearchJobId, updateSearchUiState, searchUiState} = useSearchStore.getState(); | ||
|
||
// User should NOT be able to submit a new query while an existing query is in progress. | ||
if ( | ||
searchUiState !== SEARCH_UI_STATE.DEFAULT && | ||
searchUiState !== SEARCH_UI_STATE.DONE | ||
) { | ||
console.error("Cannot submit query while existing query is in progress."); | ||
|
||
return; | ||
} | ||
|
||
updateSearchUiState(SEARCH_UI_STATE.QUERY_ID_PENDING); | ||
|
||
submitQuery(payload) | ||
.then((result) => { | ||
const {searchJobId} = result.data; | ||
|
||
updateSearchJobId(searchJobId); | ||
updateSearchUiState(SEARCH_UI_STATE.QUERYING); | ||
|
||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// eslint-disable-next-line no-warning-comments | ||
// TODO: Delete previous query results when the backend is ready | ||
|
||
Comment on lines
+38
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Keep the TODO; consider clearing stale results on cancel success too. When the backend supports it, clear prior results on both new submissions and successful cancellations to avoid showing outdated data. 🤖 Prompt for AI Agents
|
||
console.debug( | ||
"Presto search job created - ", | ||
"Search job ID:", | ||
|
@@ -24,4 +49,29 @@ const handlePrestoQuerySubmit = (payload: PrestoQueryJobCreationSchema) => { | |
}); | ||
}; | ||
|
||
export {handlePrestoQuerySubmit}; | ||
/** | ||
* Cancels an ongoing Presto search query on server. | ||
* | ||
* @param payload | ||
*/ | ||
const handlePrestoQueryCancel = (payload: PrestoQueryJobSchema) => { | ||
const {searchUiState, updateSearchUiState} = useSearchStore.getState(); | ||
if (searchUiState !== SEARCH_UI_STATE.QUERYING) { | ||
console.error("Cannot cancel query if there is no ongoing query."); | ||
|
||
return; | ||
} | ||
|
||
updateSearchUiState(SEARCH_UI_STATE.DONE); | ||
cancelQuery(payload) | ||
.then(() => { | ||
console.debug("Query cancelled successfully"); | ||
}) | ||
.catch((err: unknown) => { | ||
console.error("Failed to cancel query:", err); | ||
}); | ||
}; | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export { | ||
handlePrestoQueryCancel, handlePrestoQuerySubmit, | ||
}; |
Uh oh!
There was an error while loading. Please reload this page.