fix(ui): Avoid auto-applying LIMIT to non-SELECT statements#26987
fix(ui): Avoid auto-applying LIMIT to non-SELECT statements#26987tdcmeehan merged 2 commits intoprestodb:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefines the SQL detection logic in the Presto Web UI so that the automatic LIMIT is only applied to top-level SELECT statements, avoiding modification of CTAS, INSERT, and other non-SELECT queries that embed SELECTs. Sequence diagram for applying default LIMIT only to top level SELECTsequenceDiagram
actor User
participant WebUI
participant SQLInputModule
participant SelectListener
User->>WebUI: submit SQL text
WebUI->>SQLInputModule: sqlCleaning(sql, errorHandler)
SQLInputModule->>SelectListener: new SelectListener()
SQLInputModule->>SelectListener: walk parse tree
SelectListener->>SelectListener: enterStatement(ctx)
SelectListener->>SelectListener: set isTopLevelSelect based on ctx.query()
SQLInputModule->>SQLInputModule: inspect isTopLevelSelect, limit, fetchFirstNRows
alt isTopLevelSelect is true and limit exceeds 100
SQLInputModule->>SQLInputModule: replace LIMIT with limit 100
else isTopLevelSelect is false
SQLInputModule->>SQLInputModule: do not modify SQL
end
SQLInputModule-->>WebUI: cleaned SQL
WebUI-->>User: execute query and show results
Class diagram for updated SQLInput SelectListener limit detectionclassDiagram
class SelectListener {
+number limit
+number fetchFirstNRows
+boolean isSelect
+boolean isTopLevelSelect
+SelectListener()
+enterStatement(ctx)
}
class SQLInputModule {
+sqlCleaning(sql, errorHandler)
}
SQLInputModule --> SelectListener : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
isTopLevelSelectflag makesisSelectappear redundant in this listener; consider removingisSelect(and any associated logic) if it is no longer used to keep the state and detection logic simple and easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `isTopLevelSelect` flag makes `isSelect` appear redundant in this listener; consider removing `isSelect` (and any associated logic) if it is no longer used to keep the state and detection logic simple and easier to reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I’ve cleaned up the listener based on the feedback and removed the isSelect flag along with the related logic. Now that LIMIT enforcement is driven entirely by isTopLevelSelect, keeping isSelect didn’t add any value and made the state harder to follow. On the CI side, the failure is in prestocpp-linux-build-for-test appears to come from presto-native-execution (an HTTP invocation error in the native client). Since this PR only touches Web UI SQL preprocessing and doesn’t affect native execution, the failure looks unrelated. A re-run of the failed job should help confirm. |
|
Please add a release note entry, even if the appropriate is only |
yhwang
left a comment
There was a problem hiding this comment.
Nice change and thanks for the clean up.
/LGTM
|
Thank you @prabhushankar-ps! |
|
Thanks for the review and for re-running CI. |
Description
This change fixes how the Presto Web UI applies its default row limit. Previously, the UI added a LIMIT 100 to any statement that contained a SELECT clause, even when the statement itself was not a SELECT.
Because statements like CREATE TABLE AS SELECT and INSERT INTO … SELECT includes a SELECT as part of their syntax, they were incorrectly treated as interactive queries and could be silently truncated when run from the Web UI. This update ensures the default limit is applied only to top-level SELECT statements.
Motivation and Context
The Web UI applies a default row limit to help prevent users from accidentally returning very large result sets. The existing logic only checked for the presence of a SELECT clause, which caused non-SELECT statements that embed a SELECT to be handled incorrectly.
This change refines the SQL detection logic so that only plain SELECT statements are subject to the automatic limit.
Fixes #26975.
Impact
Interactive SELECT queries in the Web UI behave the same as before. CREATE TABLE AS SELECT, INSERT, and other non-SELECT statements are no longer modified by the UI. There are no changes to public APIs, server-side behavior, or performance.
Test Plan
Tested manually using the Presto Web UI.
Ran a SELECT without an explicit LIMIT and confirmed the default limit is applied. Ran a SELECT with an explicit LIMIT and confirmed it is preserved. Ran CREATE TABLE AS SELECT and INSERT INTO … SELECT statements and confirmed no LIMIT was appended.
Automated tests were not added because this change affects UI-side SQL preprocessing and there is no existing test coverage in this area.
Contributor checklist
Follows the Presto contributing guide and code style
PR description clearly explains the problem and why the change is needed
Linked to the relevant GitHub issue
No new public APIs, SQL syntax, or configuration changes
Manual testing completed; automated tests not applicable
CI passed (pending)
No new dependencies added