Fixed an issue where EXPLAIN and EXPLAIN ANALYZE not working if blank line is there in the SQL query.#9875
Open
Fixed an issue where EXPLAIN and EXPLAIN ANALYZE not working if blank line is there in the SQL query.#9875
Conversation
… line is there in the SQL query.
line is there in the SQL query. #9670 Use the syntax tree as the primary check for detecting when a blank-line boundary cuts through a SQL statement. If a Statement node straddles startPos (starts before AND ends after), the blank line split a clause and the query must be expanded past the blank line. A keyword list (STATEMENT_STARTERS) is retained as a secondary guard for the case where the parser merges semicolon-less queries into one Statement node — blank lines before statement-starting keywords remain legitimate separators. Key fix: Lezer's tree.iterate is inclusive at boundaries (visits nodes whose .to === from), so added a node.to > startPos check to prevent false positives from the previous Statement touching the boundary. Refs #2431, #6308
WalkthroughgetQueryAt is refactored into two internal helpers: one that locates initial start/end positions via the syntax tree while skipping comments and optionally stopping at blank lines, and another that detects when the range needs expansion across blank-line-fragmented Statement nodes, causing recomputation if required. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js (2)
27-36:STATEMENT_STARTERSis comprehensive for plain SQL, but conflates two very different categories of keywords.Keywords like
SELECT,INSERT,CREATE,DROP,COMMITgenuinely start a self-contained statement. Keywords likeEXPLAIN,ANALYZE(asEXPLAIN ANALYZE), andWITHare wrappers that must be followed by another statement. Treating them identically in_needsExpansionis the root cause of the cursor-on-EXPLAIN bug flagged above.Consider splitting into two sets (see suggested fix in the
_needsExpansioncomment), or documenting clearly that this list is intentionally a best-effort heuristic and why some wrappers still need expansion in the subsequent logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js` around lines 27 - 36, STATEMENT_STARTERS currently mixes true statement starters and wrapper keywords (e.g., EXPLAIN, ANALYZE, WITH), causing _needsExpansion to misclassify cursor positions; split the list into two distinct sets (e.g., STATEMENT_STARTERS for self-contained starters and WRAPPER_KEYWORDS for wrappers like EXPLAIN/ANALYZE/WITH) and update the _needsExpansion function to treat WRAPPER_KEYWORDS as requiring a following statement (or delegation to existing expansion logic) while keeping STATEMENT_STARTERS as standalone starters; update inline comments/docs near STATEMENT_STARTERS and _needsExpansion to explain the distinction so future maintainers understand the heuristic.
110-114: Minor: regex character class has redundancy and comment-only detection is trivially bypassed by leading whitespace in mid-range scenarios.
- The split pattern
/[\s\n\r(;]+/is redundant —\salready matches\n,\r,\t, space, etc. Prefer/[\s(;]+/.query.startsWith('--') || query.startsWith('/*')aftertrim()works for the common case, but won't catch a range whose trimmed first token is a comment followed by real SQL on later lines (extremely rare in practice given how_findQueryBoundariessegments comments, but worth a short note in the comment above to explain why the simple check is sufficient).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js` around lines 110 - 114, The current comment-only detection and token split are fragile: change the startsWith check to operate on a trimmed prefix (e.g., use query.trimStart() before checking startsWith('--') or startsWith('/*')) so leading whitespace doesn't bypass the comment-only detection, and simplify the split regex in the firstWord extraction from /[\s\n\r(;]+/ to /[\s(;]+/ since \s already includes newlines; update the code around firstWord, query.split and STATEMENT_STARTERS to use these adjustments and add a short inline comment near the check explaining that this lightweight comment-only detection is sufficient because _findQueryBoundaries already segments queries and thus fully-commented ranges are expected to be rare.web/regression/javascript/components/CodeMirrorCustomEditor.spec.js (1)
249-257: Weak assertion — test documents behaviour but doesn't actually verify anything useful.
expect(stmts.length).toBeGreaterThanOrEqual(1)will pass for virtually any non-empty input. If the intent is purely diagnostic (documenting what the parser does for future reference), consider usingconsole.login a local script or adding explicit assertions for both observed shapes so a change in parser behaviour is detected:- // Record whether parser merges or separates — our _needsExpansion - // must handle both cases correctly. The getQueryAt tests above - // already verify correct output; this test documents the parser's - // actual behavior for future reference. - expect(stmts.length).toBeGreaterThanOrEqual(1); + // Parser may merge (1 Statement) or separate (2 Statements) — both are handled by + // _needsExpansion, but pin down the exact current behaviour so a parser update is noticed. + expect([1, 2]).toContain(stmts.length); + if (stmts.length === 1) { + expect(stmts[0].from).toBe(0); + expect(stmts[0].to).toBe(41); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/regression/javascript/components/CodeMirrorCustomEditor.spec.js` around lines 249 - 257, The test uses a weak assertion (expect(stmts.length).toBeGreaterThanOrEqual(1)) that won't catch parser regressions; update the test "parser: two queries without semicolons — verify Statement layout" to explicitly assert both possible parser behaviors by inspecting getStatementNodes(editor) after calling cmRerender({value: 'SELECT * FROM users\n\nSELECT * FROM orders'}): either assert stmts.length === 2 and that the two statements correspond to the two queries, or assert stmts.length === 1 and that the single statement spans both queries (or implement a branching assertion that checks if stmts.length === 1 then assert merged span/contents, else assert two separate nodes), optionally adding a console.debug/log to surface the observed shape during test runs; refer to cmRerender, getStatementNodes, and editor to locate the test logic to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js`:
- Around line 81-115: The expansion check in _needsExpansion only detects
Statements that straddle startPos and ignores Statements that straddle endPos,
and also misclassifies wrapper keywords like EXPLAIN/ANALYZE/WITH; update the
tree.iterate logic in _needsExpansion to mark a spanning statement when
node.from < startPos && node.to > startPos OR node.from < endPos && node.to >
endPos (i.e., spans either boundary) instead of only the start-boundary check,
and then replace the STATEMENT_STARTERS heuristic with a two-tier check: keep
STATEMENT_STARTERS for true standalone-starters but introduce a WRAPPER_KEYWORDS
set (including EXPLAIN, ANALYZE, WITH) that always force expansion (treat them
as non-standalone), using the computed span flag and the firstWord to decide
expansion.
In `@web/regression/javascript/components/CodeMirrorCustomEditor.spec.js`:
- Around line 149-162: Add tests that place the cursor before the first blank
line in EXPLAIN queries to ensure getQueryAt returns the full query (guarding
_needsExpansion behavior); specifically, for both query variants used in the
file ('EXPLAIN ANALYZE SELECT *\n\nFROM...' and 'EXPLAIN SELECT *\n\nFROM...'),
call cmRerender({value: query}) and assert editor.getQueryAt(pos) returns the
full query when pos is at the start of the buffer (cursor on "EXPLAIN") and when
pos is on the "SELECT" token (i.e. immediately after "EXPLAIN " / "EXPLAIN
ANALYZE "), mirroring the existing test structure and expectations.
---
Nitpick comments:
In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js`:
- Around line 27-36: STATEMENT_STARTERS currently mixes true statement starters
and wrapper keywords (e.g., EXPLAIN, ANALYZE, WITH), causing _needsExpansion to
misclassify cursor positions; split the list into two distinct sets (e.g.,
STATEMENT_STARTERS for self-contained starters and WRAPPER_KEYWORDS for wrappers
like EXPLAIN/ANALYZE/WITH) and update the _needsExpansion function to treat
WRAPPER_KEYWORDS as requiring a following statement (or delegation to existing
expansion logic) while keeping STATEMENT_STARTERS as standalone starters; update
inline comments/docs near STATEMENT_STARTERS and _needsExpansion to explain the
distinction so future maintainers understand the heuristic.
- Around line 110-114: The current comment-only detection and token split are
fragile: change the startsWith check to operate on a trimmed prefix (e.g., use
query.trimStart() before checking startsWith('--') or startsWith('/*')) so
leading whitespace doesn't bypass the comment-only detection, and simplify the
split regex in the firstWord extraction from /[\s\n\r(;]+/ to /[\s(;]+/ since \s
already includes newlines; update the code around firstWord, query.split and
STATEMENT_STARTERS to use these adjustments and add a short inline comment near
the check explaining that this lightweight comment-only detection is sufficient
because _findQueryBoundaries already segments queries and thus fully-commented
ranges are expected to be rare.
In `@web/regression/javascript/components/CodeMirrorCustomEditor.spec.js`:
- Around line 249-257: The test uses a weak assertion
(expect(stmts.length).toBeGreaterThanOrEqual(1)) that won't catch parser
regressions; update the test "parser: two queries without semicolons — verify
Statement layout" to explicitly assert both possible parser behaviors by
inspecting getStatementNodes(editor) after calling cmRerender({value: 'SELECT *
FROM users\n\nSELECT * FROM orders'}): either assert stmts.length === 2 and that
the two statements correspond to the two queries, or assert stmts.length === 1
and that the single statement spans both queries (or implement a branching
assertion that checks if stmts.length === 1 then assert merged span/contents,
else assert two separate nodes), optionally adding a console.debug/log to
surface the observed shape during test runs; refer to cmRerender,
getStatementNodes, and editor to locate the test logic to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8872718-fcb1-4315-9731-3ae1b7eb4821
📒 Files selected for processing (2)
web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.jsweb/regression/javascript/components/CodeMirrorCustomEditor.spec.js
Address CodeRabbit review: the expansion check was asymmetric — it only detected Statements straddling startPos but missed Statements extending past endPos. This caused EXPLAIN queries to be truncated when cursor was on the EXPLAIN keyword itself. Changes: - Added statementExtendsPastEnd check alongside statementStraddlesStart - Introduced WRAPPER_STARTERS (EXPLAIN, ANALYZE, WITH) that always force expansion when the Statement extends past the extracted range - Simplified split regex (\s already includes \n and \r) - Strengthened weak parser diagnostic test assertion - Added tests: cursor on EXPLAIN keyword, cursor on SELECT in EXPLAIN ANALYZE All 845 JS tests pass (140 suites, 36 CodeMirror-specific).
Contributor
Author
|
Addressed all CodeRabbit findings in 438efea: Major fix — asymmetric expansion check:
Nitpicks fixed:
New tests added:
All 845 JS tests pass (140 suites, 36 CodeMirror-specific). |
Contributor
Author
|
Responding to CodeRabbit nitpick comments from the review: Nitpick 1 — Split STATEMENT_STARTERS into starters + wrappers: Nitpick 2 — Regex redundancy and comment detection: Nitpick 3 — Weak parser diagnostic test: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js (2)
211-237: Optional cleanup in_findQueryBoundaries: dead guard + tautologicalif.Two minor code-hygiene items in the forward loop that don't affect correctness:
- Line 212 —
maxEndPos == -1 ? this.state.doc.length : maxEndPosis unreachable:maxEndPosis initialized tothis.state.doc.length(line 147) and is only ever reassigned tonode.toinside the loop. It's never-1.- Line 235 — the check
if(node.type.name == 'Statement')is always true at that point because Line 229 already handled andcontinue'd the non-Statement branch.Proposed cleanup
// Move forward from start position let endPos = startPos + 1; - maxEndPos = maxEndPos == -1 ? this.state.doc.length : maxEndPos; while(endPos < maxEndPos) { const currLine = this.state.doc.lineAt(endPos); // If empty line in between then that's it if(currLine.text.trim() == '' && stopAtBlankLine) { break; } let node = tree.resolve(endPos); // Skip the comments if(node.type.name == 'LineComment' || node.type.name == 'BlockComment') { endPos = node.to + 1; continue; } // Skip any other types if(node.type.name != 'Statement') { endPos += 1; continue; } // Can't go beyond a statement - if(node.type.name == 'Statement') { - maxEndPos = node.to; - } + maxEndPos = node.to; if(currLine.to < maxEndPos) { endPos = currLine.to + 1; } else { endPos += 1; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js` around lines 211 - 237, In _findQueryBoundaries, remove the dead/unreachable ternary that sets maxEndPos to this.state.doc.length when comparing against -1 (the variable maxEndPos is initialized to this.state.doc.length and never -1), and remove the redundant if(node.type.name == 'Statement') branch at the end (it's tautologically true because non-Statement nodes already continue); instead directly assign maxEndPos = node.to when you encounter a Statement node in the forward loop (the code that resolves node via tree.resolve(endPos) and the existing comment/continue logic around LineComment/BlockComment and non-Statement nodes should remain).
118-127: Add regression test for cursor at position 0 of blank-line-split single statement, or clarify intended behavior.Test coverage gap: The query
SELECT *\n\nFROM pg_class\n\nWHERE id = 1;is tested with cursor on FROM (position 10) and WHERE (position 25), both returning the full query. However, no test covers cursor at position 0 (the SELECT keyword). Cursor at position 0 currently returns"SELECT *"(syntactically incomplete SQL), whereas symmetric cursor positions return the complete query.This asymmetry may be intentional to prevent merging of two semicolon-less queries (e.g.,
SELECT * FROM users\n\nSELECT * FROM orderswith cursor on first SELECT should not expand). However, without a regression test pinning down the cursor-at-pos-0 case for a truly single statement, future changes to the expansion heuristic could silently flip this behavior. Either add a test documenting the expected behavior or, if the incomplete result is unintended, add a targeted handler in_needsExpansion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js` around lines 118 - 127, The expansion heuristic in _needsExpansion currently returns incomplete results for a cursor at position 0 on a single statement split by blank lines (it treats the SELECT as incomplete via statementExtendsPastEnd/firstWord checks), so either add a regression test asserting the expected behavior for the query "SELECT *\n\nFROM pg_class\n\nWHERE id = 1;" with cursor position 0 (to lock in the current behavior) or change _needsExpansion to treat a cursor at document start differently: detect cursor at position 0 and if the overall parse shows there is no second semicolon-separated statement (i.e., the blank-line split is within one logical statement), return true (expand) instead of relying solely on WRAPPER_STARTERS/STATEMENT_STARTERS; reference the _needsExpansion function and its locals statementExtendsPastEnd, statementStraddlesStart, firstWord, WRAPPER_STARTERS and STATEMENT_STARTERS when adding the test or adding the targeted special-case handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.js`:
- Around line 211-237: In _findQueryBoundaries, remove the dead/unreachable
ternary that sets maxEndPos to this.state.doc.length when comparing against -1
(the variable maxEndPos is initialized to this.state.doc.length and never -1),
and remove the redundant if(node.type.name == 'Statement') branch at the end
(it's tautologically true because non-Statement nodes already continue); instead
directly assign maxEndPos = node.to when you encounter a Statement node in the
forward loop (the code that resolves node via tree.resolve(endPos) and the
existing comment/continue logic around LineComment/BlockComment and
non-Statement nodes should remain).
- Around line 118-127: The expansion heuristic in _needsExpansion currently
returns incomplete results for a cursor at position 0 on a single statement
split by blank lines (it treats the SELECT as incomplete via
statementExtendsPastEnd/firstWord checks), so either add a regression test
asserting the expected behavior for the query "SELECT *\n\nFROM
pg_class\n\nWHERE id = 1;" with cursor position 0 (to lock in the current
behavior) or change _needsExpansion to treat a cursor at document start
differently: detect cursor at position 0 and if the overall parse shows there is
no second semicolon-separated statement (i.e., the blank-line split is within
one logical statement), return true (expand) instead of relying solely on
WRAPPER_STARTERS/STATEMENT_STARTERS; reference the _needsExpansion function and
its locals statementExtendsPastEnd, statementStraddlesStart, firstWord,
WRAPPER_STARTERS and STATEMENT_STARTERS when adding the test or adding the
targeted special-case handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c922c255-8234-4f8c-ae89-5370f915977f
📒 Files selected for processing (2)
web/pgadmin/static/js/components/ReactCodeMirror/CustomEditorView.jsweb/regression/javascript/components/CodeMirrorCustomEditor.spec.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
startPos(starts before AND ends after), the blank line split a clause and the query is expanded past the blank lineSTATEMENT_STARTERS) is retained as a secondary guard for the case where the parser merges semicolon-less queries into one Statement nodetree.iterateis inclusive at boundaries, addednode.to > startPoscheck to prevent false positivesSupersedes #9670
Refs #2431, #6308
Test plan
Summary by CodeRabbit
Bug Fixes
Refactor
Tests