Support Oracle-style SQL block splitting#325
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Previous Issues ResolvedAll 7 issues from the previous review have been addressed:
Files Reviewed (5 files)
Previous Review Summary (commit 6ee4d24)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6ee4d24)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by kimi-k2.6-20260420 · Input: 57K · Output: 17.8K · Cached: 189.4K |
|
Thanks @haos666 for the follow-up — happy to take a look! |
| case 'PACKAGE': | ||
| case 'TYPE': | ||
| case 'CLASS': | ||
| return next === 'BODY' || next !== undefined; |
There was a problem hiding this comment.
[should-fix] CREATE TYPE (spec) and CREATE LIBRARY are mis-classified as PL/SQL block openers, so foldBlocks swallows the following statement when no / is present.
return next === 'BODY' || next !== undefined reduces to next !== undefined (the first clause is dead), so any CREATE TYPE <name> is an opener; LIBRARY returns true unconditionally at L201. Object-type specs and CREATE LIBRARY carry no internal ; — the ; is their terminator — so folding them has no upside and merges the next statement. Tabularis sends statements to a driver one-by-one (no /, no trailing ;), so a merged CREATE TYPE ...; SELECT ... fails (ORA-00911) and the SELECT is lost.
The right criterion is "does it contain internal ;": PACKAGE/CLASS specs do, TYPE spec / LIBRARY don't.
case 'FUNCTION': case 'PROCEDURE': case 'TRIGGER':
return true;
case 'LIBRARY':
return false; // one-line DDL, terminated by ;
case 'PACKAGE': case 'CLASS':
return next !== undefined; // specs carry internal ;
case 'TYPE':
return next === 'BODY'; // only TYPE BODY carries internal ;A blanket next === 'BODY' for all three breaks the existing DM CREATE CLASS <name> spec test, so keep PACKAGE/CLASS as next !== undefined. Please add regressions: CREATE TYPE ...; SELECT ... -> 2 statements, CREATE LIBRARY ...; SELECT ... -> 2.
| ) { | ||
| index++; | ||
| } | ||
| if (tokens[index] === 'AND' && isJavaCompileOption(tokens[index + 1])) { |
There was a problem hiding this comment.
Oracle grammar is CREATE [OR REPLACE] [AND {RESOLVE|COMPILE}] [NOFORCE] JAVA {SOURCE|CLASS|RESOURCE} .... Only AND RESOLVE|COMPILE is skipped here; the optional NOFORCE that follows is not, so CREATE OR REPLACE AND RESOLVE NOFORCE JAVA SOURCE ... falls through to default and the Java body splits on its internal ;. Add if (tokens[index] === 'NOFORCE') index++; after the AND-skip. Low frequency, but a real grammar gap.
| } | ||
|
|
||
| function isPlsqlBlockOpener(sql: string, segment: RawSegment): boolean { | ||
| const tokens = leadingSignificantTokens(sql.slice(segment.start, segment.end)); |
There was a problem hiding this comment.
sql.slice(segment.start, segment.end) copies the whole segment (a package body can be tens of KB) even though leadingSignificantTokens only needs the opener prefix. This is avoidable GC pressure on the parse path.
I would not fix this by capping the slice to a fixed byte/char count, because a segment can start with a long license/comment block and still have a valid opener later. A safer shape is to let leadingSignificantTokens scan the original sql with start/end bounds, stopping once it has collected enough tokens.
Same function, minor: the 12 cap deserves a named const/comment, and source.slice(position) in the word loop allocates per token. A sticky /[A-Za-z_][A-Za-z0-9_$#]*/y (matching the existing DOLLAR_TAG_RE/GO_RE/SLASH_RE convention) avoids that allocation.
| const endSegment = segments[endIndex]; | ||
| output.push({ | ||
| start: segment.start, | ||
| end: endSegment.end, |
There was a problem hiding this comment.
When no / is found, the merged span ends at endSegment.end, which is before the segment terminator. For a block ending exactly at EOF, END; becomes END in both Statement.text and Statement.range.
The current backend sanitization (sanitize_user_query -> trim_end_matches(';'), src-tauri/src/commands.rs:89) masks the driver impact for now because it strips that same trailing semicolon before execution anyway. The splitter should still preserve the original statement text/range; otherwise this becomes an execution bug as soon as semicolon stripping is made statement-aware.
let eofFold = false;
if (endIndex >= segments.length) {
endIndex = segments.length - 1;
eofFold = true;
}
// ...
end: eofFold ? sql.length : endSegment.end,| const openCodePoint = source.codePointAt(delimiterPosition); | ||
| if (openCodePoint === undefined) return null; | ||
|
|
||
| const openDelimiter = String.fromCodePoint(openCodePoint); |
There was a problem hiding this comment.
Per Oracle, a q-quote delimiter cannot be a space, tab, or newline; these are accepted here, so q' ...;... ' would be mis-scanned as a quoted literal. Low impact (only over-shields malformed input), but a guard rejecting whitespace delimiters would match the spec — note a single quote is a valid delimiter, so don't reject '. The UTF-16/codePoint handling and the identifier-boundary guard here look correct.
| readonly dollarQuoting: boolean; | ||
| readonly customDelimiter: boolean; | ||
| readonly goDelimiter: boolean; | ||
| readonly slashTerminator: boolean; |
There was a problem hiding this comment.
Nit: the option flag is slashTerminator but the token kind / SegmentTerminator value is slashDelimiter. The pre-existing goDelimiter uses one word for both the flag and the token. Aligning to slashDelimiter (flag + the tokenizer.ts guard) keeps the grep trail consistent.
| ); | ||
| const result = splitQueries(sql, 'oracle'); | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0]).toContain('NULL;'); |
There was a problem hiding this comment.
This asserts NULL; but not the trailing END;, so the EOF semicolon loss (see the foldBlocks comment) passes unnoticed. Add expect(result[0]).toContain('END;'). Also worth adding: CREATE TYPE ...; SELECT ... and CREATE LIBRARY ...; SELECT ... -> length 2 (opener fix), CREATE TYPE BODY ... / -> length 1, WITH PROCEDURE ..., and a CRLF / separator case.
|
Thanks @haos666 — clean, well-tested change, and the dialect gating looks solid: the new I went through this carefully and checked the Oracle semantics against the Oracle SQL reference and the node-oracledb docs. The one correctness item I'd like resolved before merge is inline: Out of scope of this PR's files, but they block PL/SQL execution end-to-end through the UI (NOT regressions from this PR — flagging to track separately, probably as their own issues):
The MCP path ( |
|
Thanks for the detailed review. I pushed an update addressing the requested splitter fixes:
Validation run locally:
I also verified the updated dev app UI against a local DM connection: the query chooser now splits the reviewed cases as expected, including TYPE/LIBRARY as two statements and trigger/Java blocks plus following SELECT as two statements. |
ymadd
left a comment
ymadd
left a comment
There was a problem hiding this comment.
LGTM. This addresses all my earlier review points — rename to slashDelimiter, leadingSignificantTokens substring removal + sticky WORD_RE, EOF fold preserving the trailing ;, TYPE/PACKAGE/CLASS split, NOFORCE + JAVA RESOURCE, q-quote whitespace-delimiter guard, and CRLF separator coverage. @debba good to merge from my side.
One non-blocking edge case I'll note for the record (no action needed here): a type spec whose name tokenizes to BODY — CREATE TYPE "BODY" AS OBJECT (...), or unquoted CREATE TYPE body ... since BODY isn't a SQL reserved word — is mis-folded as a TYPE BODY opener, so a following statement gets merged into it. The trigger is pathological, so it's not worth holding this PR. I'll fold the fix (a lookahead so a BODY-named spec doesn't satisfy the body pattern, plus a regression test) into a later splitter change.
|
That's great. |
3 participants
Summary
Follow-up to #316 for the Oracle/DM splitter work discussed with @ymadd.
This keeps the behavior additive and dialect-gated behind the existing
oracleSQL dialect:/as an Oracle-style statement terminator;and/inside quoted literals do not split statementsCREATE CLASS,CREATE CLASS BODY, andCREATE JAVA CLASSoracleThis PR does not include trigger RPC forwarding (#321), batch forwarding, SQL import/export, or BLOB hooks.
Validation
Automated checks:
pnpm exec vitest run tests/utils/sqlSplitterpnpm exec vitest run tests/utils/sqlSplitter/dialects.test.ts tests/utils/sqlSplitter/splitter.test.ts tests/utils/sqlSplitter/tokenizer.test.tspnpm exec vitest run(130 files / 2588 tests)pnpm run typecheckpnpm run lintpnpm run buildpnpm run test:rust(671 Rust tests)git diff --checkLocal DM/UI smoke on the DM external plugin:
CREATE OR REPLACE TRIGGER ... BEGIN ... END; /plus a followingSELECTis detected as 2 statements, not split at internal semicolons.;and a line-leading/stays inside one statement.oracledialect.