Skip to content

Add Oracle PL/SQL structural parsing - #785

Open
satwikjambula wants to merge 21 commits into
tirth8205:stagingfrom
satwikjambula:feature/oracle-plsql-support
Open

satwikjambula wants to merge 21 commits into
tirth8205:stagingfrom
satwikjambula:feature/oracle-plsql-support

Conversation

@satwikjambula

Copy link
Copy Markdown

Adds support for Oracle PL/SQL constructs that tree-sitter-sql cannot parse (IS/AS ... BEGIN ... END bodies with no RETURNS/LANGUAGE clause parse as ERROR nodes under the ANSI-oriented grammar), via the same regex-fallback approach already used for CREATE PROCEDURE:

  • Route Oracle-specific extensions (.pls, .plb, .pks, .pkb, .pck, .prc, .fnc, .trg) through the existing SQL parser dispatch.
  • Extract CREATE PACKAGE / PACKAGE BODY (with nested PROCEDURE/FUNCTION members qualified as pkg_name.member_name), CREATE TRIGGER, and Oracle-style CREATE FUNCTION/PROCEDURE.
  • Add regex-based CALLS edge extraction within procedure/function/ trigger/package-member bodies, skipping PL/SQL keywords and scalar type names that also precede '(' (e.g. VARCHAR2(50) declarations).
  • Add tests/fixtures/sample_plsql.sql and a TestPLSQLParsing suite covering packages, triggers, nested members, and call edges.
  • Update README/docs language lists and CHANGELOG.

Pull Request

Linked issue

Closes #

What & why

How it was tested

uv run pytest tests/ --tb=short -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional

Checklist

  • Tests added for new functionality
  • All tests pass: uv run pytest tests/ --tb=short -q
  • Linting passes: uv run ruff check code_review_graph/
  • Type checking passes: uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
  • Lines are at most 100 characters
  • Docs updated where behavior changed (README, docs/, docstrings)

Adds support for Oracle PL/SQL constructs that tree-sitter-sql cannot
parse (IS/AS ... BEGIN ... END bodies with no RETURNS/LANGUAGE clause
parse as ERROR nodes under the ANSI-oriented grammar), via the same
regex-fallback approach already used for CREATE PROCEDURE:

- Route Oracle-specific extensions (.pls, .plb, .pks, .pkb, .pck, .prc,
  .fnc, .trg) through the existing SQL parser dispatch.
- Extract CREATE PACKAGE / PACKAGE BODY (with nested PROCEDURE/FUNCTION
  members qualified as pkg_name.member_name), CREATE TRIGGER, and
  Oracle-style CREATE FUNCTION/PROCEDURE.
- Add regex-based CALLS edge extraction within procedure/function/
  trigger/package-member bodies, skipping PL/SQL keywords and scalar
  type names that also precede '(' (e.g. VARCHAR2(50) declarations).
- Add tests/fixtures/sample_plsql.sql and a TestPLSQLParsing suite
  covering packages, triggers, nested members, and call edges.
- Update README/docs language lists and CHANGELOG.
@tirth8205

Copy link
Copy Markdown
Owner

Thanks - the problem is real (verified: tree-sitter-sql emits ERROR nodes for IS/AS...BEGIN bodies, and .pkb/.pls files were skipped entirely on main), the happy path works, and the full suite passes locally (2317 passed). Four issues need fixing before merge: 1) Non-Oracle regression: for T-SQL/MySQL procedures with no 'END;' (bare END + GO, or END$$), _plsql_block_end falls back to len(text), so procedures now span to EOF and pick up false CALLS edges across the rest of the file (I observed CALLS to VARCHAR, DATEADD, GETDATE, and a later table from both procedures in a two-proc T-SQL file). Please fall back to the old single-line span (and skip call extraction) when no END terminator is found. 2) Forward declarations in package bodies ('PROCEDURE log_event(p IN VARCHAR2);') create a phantom member whose block-end lands inside the next member, emitting false CALLS edges (log_event -> total_orders in my test). Require IS/AS after the signature before treating a member as a definition. 3) Call noise: 'INSERT INTO t (cols)' emits CALLS to the table, and builtins COUNT/NVL/TO_CHAR/SUBSTR become CALLS targets; plain VARCHAR also leaks (keyword list has varchar2 only). Consider skipping identifiers preceded by INTO and adding common builtins/types to the skip list. 4) Package spec and body both produce Class 'file::pkg' which collide on the nodes UNIQUE(qualified_name) upsert, so one is lost. Also please add tests for bare 'END;' members, a forward declaration, and a T-SQL non-regression case - the current fixture only uses named 'END name;' blocks.

claude and others added 2 commits July 30, 2026 21:18
Fixes from the PR tirth8205#785 review:

- Non-Oracle regression: body-span computation and CALLS extraction now
  only run when _is_plsql_dialect confirms the file is Oracle PL/SQL (an
  Oracle extension, or a content signal like VARCHAR2/%TYPE/:NEW.). A
  plain T-SQL/MySQL CREATE PROCEDURE with no END; terminator previously
  fell back to end-of-file and picked up false CALLS edges spanning
  unrelated code; it now gets the original single-line Function node
  with no computed span, same as before this file had any PL/SQL
  handling. This also gives PL/SQL a clean separation from generic
  ANSI/T-SQL/MySQL SQL, which was mixed under one untriaged code path.
- Forward declarations: PROCEDURE/FUNCTION member declarations with no
  body (`PROCEDURE x(...);`) are now detected and skipped instead of
  being treated as a real member whose block-end search lands on the
  next member's END, producing a phantom node and false CALLS edges.
- Call noise: added the missing VARCHAR (only VARCHAR2 was excluded)
  and other scalar types to the keyword blocklist; excluded the target
  of INSERT INTO t (cols); and excluded common builtins (COUNT, NVL,
  TO_CHAR, SUBSTR, ...) and DBMS_*/UTL_* qualified builtin package calls.
- Duplicate nodes: package body now gets a qualified name distinct from
  its spec (pkg_name$body vs pkg_name) since they're separate Oracle
  objects that previously collided on nodes.qualified_name UNIQUE.

Also added the requested regression tests: bare END; members, forward
declarations, and T-SQL non-regression.
GraphStore.upsert_node computes each node's real qualified_name from
identity_name/parent_name; it never reads the local body_qualified/
member_qualified strings this parser builds for edge targets. The
previous fix only set those local strings, so the persisted package
body node still collided with its spec (both f"{file}::{pkg_name}"),
and CONTAINS/CALLS edges pointed at a qualified name no stored node
actually had. Thread the $body suffix through identity_name (package
body node) and parent_name (its members) so the real persisted
qualified name matches. Verified with GraphStore directly, and added
a regression test that goes through the real store instead of only
checking raw parser output.
@satwikjambula

Copy link
Copy Markdown
Author

Thanks - the problem is real (verified: tree-sitter-sql emits ERROR nodes for IS/AS...BEGIN bodies, and .pkb/.pls files were skipped entirely on main), the happy path works, and the full suite passes locally (2317 passed). Four issues need fixing before merge: 1) Non-Oracle regression: for T-SQL/MySQL procedures with no 'END;' (bare END + GO, or END$$), _plsql_block_end falls back to len(text), so procedures now span to EOF and pick up false CALLS edges across the rest of the file (I observed CALLS to VARCHAR, DATEADD, GETDATE, and a later table from both procedures in a two-proc T-SQL file). Please fall back to the old single-line span (and skip call extraction) when no END terminator is found. 2) Forward declarations in package bodies ('PROCEDURE log_event(p IN VARCHAR2);') create a phantom member whose block-end lands inside the next member, emitting false CALLS edges (log_event -> total_orders in my test). Require IS/AS after the signature before treating a member as a definition. 3) Call noise: 'INSERT INTO t (cols)' emits CALLS to the table, and builtins COUNT/NVL/TO_CHAR/SUBSTR become CALLS targets; plain VARCHAR also leaks (keyword list has varchar2 only). Consider skipping identifiers preceded by INTO and adding common builtins/types to the skip list. 4) Package spec and body both produce Class 'file::pkg' which collide on the nodes UNIQUE(qualified_name) upsert, so one is lost. Also please add tests for bare 'END;' members, a forward declaration, and a T-SQL non-regression case - the current fixture only uses named 'END name;' blocks.

Thanks for the thorough review. All four issues were valid and reproducible, so I really appreciate you catching them.

I’ve addressed each one in the latest commits:

Non-Oracle regression: I added dialect detection (_is_plsql_dialect) so every PL/SQL-specific pass—body span calculation, CALLS extraction, packages, and triggers—only runs when the file is actually Oracle PL/SQL. The check uses either the file extension or Oracle-specific content signals such as VARCHAR2, %TYPE, or :NEW. As a result, a T-SQL or MySQL CREATE PROCEDURE without an END; now behaves exactly as it did before this PR: it remains a single-line node with no computed span, rather than incorrectly expanding to the end of the file.

Forward declarations: I added _plsql_is_forward_decl to detect signature-only declarations such as PROCEDURE x(...); inside package bodies. These are now skipped, so they’re no longer treated as real members whose block-end search can accidentally attach to the next member’s END.

Call extraction noise: I expanded the filtering logic by excluding VARCHAR (previously only VARCHAR2 was excluded), adding a blocklist for common built-in functions (COUNT, NVL, TO_CHAR, SUBSTR, etc.), filtering out DBMS_* and UTL_* package calls, and excluding the target table in INSERT INTO t (...) statements.

Duplicate nodes: Package bodies now receive a distinct qualified name (pkg_name$body) instead of sharing the same name as the package specification. While validating this against the real GraphStore (not just the parser output), I also found a related bug: GraphStore.upsert_node derives the persisted qualified name from identity_name and parent_name, independently of the parser’s local edge-target strings. The fix therefore had to be applied through those fields, not just within _parse_sql. I added a regression test that exercises the actual store layer so this class of issue can’t slip through unnoticed again.

I also added the requested test coverage for:

bare END; members,

forward declarations, and

the T-SQL non-regression case.

I ran the full suite (2,400+ tests, including the upstream tests), along with ruff and mypy, and everything passes. I also verified that the branch merges cleanly against the current main.

@github-actions

github-actions Bot commented Jul 30, 2026 •

Copy link
Copy Markdown

code-review-graph review

Overall risk: 0.60 (MEDIUM) — 60 changed function(s)/class(es), 17 affected flow(s), 8 test gap(s)

Risk-scored changes

Risk Level Symbol Location Tested
0.60 medium code_review_graph/parser.py::CodeParser._plsql_member_signature code_review_graph/parser.py:5194 yes
0.55 medium code_review_graph/parser.py::CodeParser._is_plsql_dialect code_review_graph/parser.py:5067 yes
0.55 medium code_review_graph/parser.py::CodeParser._strip_plsql_noise code_review_graph/parser.py:5086 no
0.50 medium code_review_graph/parser.py::CodeParser._plsql_is_forward_decl code_review_graph/parser.py:5142 yes
0.50 medium code_review_graph/parser.py::CodeParser._plsql_block_end code_review_graph/parser.py:5707 yes
0.50 medium tests/fixtures/sample_plsql.sql::employees tests/fixtures/sample_plsql.sql:3 no
0.50 medium tests/fixtures/sample_plsql.sql::give_raise tests/fixtures/sample_plsql.sql:9 no
0.50 medium tests/test_multilang.py::TestPLSQLParsing.test_finds_table tests/test_multilang.py:3901 (test)
0.50 medium tests/test_multilang.py::TestPLSQLParsing.test_finds_procedure tests/test_multilang.py:3905 (test)
0.50 medium tests/test_multilang.py::TestPLSQLParsing.test_finds_oracle_style_function tests/test_multilang.py:3913 (test)

Affected execution flows

  • visit_FunctionDef — criticality 0.41, 55 node(s) across 1 file(s)
  • visit_AsyncFunctionDef — criticality 0.41, 55 node(s) across 1 file(s)
  • visit_ClassDef — criticality 0.41, 54 node(s) across 1 file(s)
  • visit_If — criticality 0.36, 3 node(s) across 1 file(s)
  • repo_relative_path — criticality 0.36, 9 node(s) across 1 file(s)
  • ...and 12 more affected flow(s)

Test gaps

  • code_review_graph/parser.py::CodeParser._strip_plsql_noise (code_review_graph/parser.py:5086)
  • code_review_graph/parser.py::CodeParser._blank_block_comment (code_review_graph/parser.py:5101)
  • code_review_graph/parser.py::CodeParser._parse_sql (code_review_graph/parser.py:5250)
  • code_review_graph/parser.py::CodeParser._extract_plsql_calls (code_review_graph/parser.py:5735)
  • tests/fixtures/sample_plsql.sql::employees (tests/fixtures/sample_plsql.sql:3)
  • ...and 3 more without direct tests

Token savings: this graph-backed report used ~267,895 fewer tokens (~91%) than reading every changed file in full (estimated, chars/4 approximation).


Powered by code-review-graph — local-first analysis; no code leaves the CI runner.

@tirth8205

Copy link
Copy Markdown
Owner

Re-reviewed after the update. All four blockers are verifiably fixed: the two-proc T-SQL file (bare END + GO) now keeps single-line spans with zero CALLS edges (MySQL END$$ too), the forward-declaration fixture produces exactly one log_event member with no phantom edges, the noise fixture emits only real user calls (VARCHAR, COUNT/NVL/TO_CHAR/SUBSTR, DBMS_/UTL_, and INSERT INTO targets all filtered), and spec+body both survive the real GraphStore with distinct qualified names and no dangling CONTAINS targets - the identity_name/parent_name threading matches _make_qualified. I also confirmed the new tests fail against the pre-fix parser (the T-SQL test fails with span 2..11, the exact EOF-bleed regression). Full suite 2400 passed / 5 skipped / 2 xpassed; ruff and mypy clean.

One remaining defect in the new _plsql_is_forward_decl: it tries re.match(r"(IS|AS)\b", text[i:]) at every character position, with no leading word boundary and no comment handling. Two consequences I reproduced: (1) a forward declaration like 'FUNCTION get_alias(p) RETURN t_alias;' (any depth-0 word ending in as/is, including 'RETURN employees.alias%TYPE;') matches AS mid-word and becomes a phantom member with false CALLS edges - the same failure mode as blocker 2, just on a narrower trigger; (2) a real definition with a semicolon inside a trailing comment, e.g. 'PROCEDURE setup(p IN NUMBER) -- initializes state; call once' followed by IS, is classified as a forward declaration and silently dropped from the graph. Suggested fix: only attempt the IS/AS match when the previous character is not a word character, and skip -- and /* */ comments during the scan. That plus a small test for each case and this is good to merge. (Also noted, non-blocking: the named END search in _plsql_block_end is unbounded to EOF, so same-named members across two packages can bleed spans when the first uses a bare END; - pre-existing from the original PR, fine as a follow-up.)

satwikjambula and others added 3 commits July 31, 2026 00:32
…erloads

Comment/string-literal awareness:
- Added _strip_plsql_noise to blank --/* */ comments and string-literal
  contents before every PL/SQL regex pass runs (block-end search,
  forward-declaration detection, call extraction, CREATE-statement
  matching, table-reference pass). Without it, a `;` or `END name;`
  inside a trailing comment or a dynamic-SQL string literal was
  indistinguishable from a real terminator, silently dropping members
  or truncating body spans.
- _is_plsql_dialect now runs against the cleaned text too, not just the
  passes it gates. A comment merely mentioning an Oracle word in an
  otherwise plain T-SQL/MySQL file (e.g. "-- migrated from Oracle
  VARCHAR2") was tripping dialect detection and re-enabling body-span
  computation for a dialect that never has an END; terminator.
- _plsql_is_forward_decl and _plsql_member_signature independently skip
  comments/strings too, rather than being correct only because callers
  happen to pass pre-cleaned text.

Identifier and boundary fixes:
- Oracle identifiers may contain $/# (e.g. log_event$v2); the CREATE-
  statement regexes used plain \w+ and silently truncated the name,
  which then cascaded into the block-end search looking for the wrong
  terminator.
- _plsql_is_forward_decl's IS/AS check only anchored the end of the
  match with \b, not the start, so a return type like t_alias matched
  "AS" mid-word and a real forward declaration was misclassified as a
  definition, producing a phantom member with false CALLS edges.
- Every CREATE/END-anchored regex now requires a leading word boundary:
  "RECREATE PROCEDURE ..." no longer matches as "CREATE PROCEDURE ...",
  and "ENDfoo;" no longer matches as "END foo;".

Overloaded package members:
- Oracle allows overloaded PROCEDURE/FUNCTION members within a package
  (never for standalone subprograms). Two overloads sharing a name
  collided on the graph store's UNIQUE(qualified_name), silently
  dropping one. Members are now qualified by name plus parameter
  signature (pkg_name$body.member_name(params)), mirroring this file's
  existing _cpp_function_identity convention for stable overload
  identities.

Added unit tests for each fix, both as direct calls into the affected
helpers and as full parse_file()/GraphStore integration tests. Full
suite passes; ruff and mypy clean.
@satwikjambula

satwikjambula commented Jul 31, 2026 •

Copy link
Copy Markdown
Author

Re-reviewed after the update. All four blockers are verifiably fixed: the two-proc T-SQL file (bare END + GO) now keeps single-line spans with zero CALLS edges (MySQL END$$ too), the forward-declaration fixture produces exactly one log_event member with no phantom edges, the noise fixture emits only real user calls (VARCHAR, COUNT/NVL/TO_CHAR/SUBSTR, DBMS__/UTL__, and INSERT INTO targets all filtered), and spec+body both survive the real GraphStore with distinct qualified names and no dangling CONTAINS targets - the identity_name/parent_name threading matches _make_qualified. I also confirmed the new tests fail against the pre-fix parser (the T-SQL test fails with span 2..11, the exact EOF-bleed regression). Full suite 2400 passed / 5 skipped / 2 xpassed; ruff and mypy clean.

One remaining defect in the new _plsql_is_forward_decl: it tries re.match(r"(IS|AS)\b", text[i:]) at every character position, with no leading word boundary and no comment handling. Two consequences I reproduced: (1) a forward declaration like 'FUNCTION get_alias(p) RETURN t_alias;' (any depth-0 word ending in as/is, including 'RETURN employees.alias%TYPE;') matches AS mid-word and becomes a phantom member with false CALLS edges - the same failure mode as blocker 2, just on a narrower trigger; (2) a real definition with a semicolon inside a trailing comment, e.g. 'PROCEDURE setup(p IN NUMBER) -- initializes state; call once' followed by IS, is classified as a forward declaration and silently dropped from the graph. Suggested fix: only attempt the IS/AS match when the previous character is not a word character, and skip -- and /* */ comments during the scan. That plus a small test for each case and this is good to merge. (Also noted, non-blocking: the named END search in _plsql_block_end is unbounded to EOF, so same-named members across two packages can bleed spans when the first uses a bare END; - pre-existing from the original PR, fine as a follow-up.)

Both fixed.

Mid-word AS/IS match — re.match(r"(IS|AS)\b", text[i:]) only anchored the end of the match with \b; there was no check on the start. Added a boundary check so the regex is only attempted when the previous character isn't a word character:

if c.isalpha() and (i == start or not (text[i-1].isalnum() or text[i-1] == "_")):

Verified with your exact repro — FUNCTION get_alias(p) RETURN t_alias; is now correctly classified as a forward declaration, and the employees.alias%TYPE variant too.

Comment before IS — added --//* */ comment-skipping directly inside _plsql_is_forward_decl itself (not just relying on the clean_text call site upstream), so the function is correct standalone. Your exact repro (PROCEDURE setup(p IN NUMBER) -- initializes state; call once followed by IS) is now correctly classified as a real definition.

Added a unit test for each case (calling _plsql_is_forward_decl directly) plus a parse_file() integration test for the t_alias scenario. While auditing for the same failure pattern elsewhere, found and fixed three more instances: _plsql_block_end matched "ENDfoo;" as "END foo;" (no boundary after END), the CREATE-statement regexes matched "RECREATE PROCEDURE" as "CREATE PROCEDURE" (no leading boundary), and _plsql_member_signature could get confused by an unbalanced paren inside a string-literal default value. All now boundary-anchored and independently comment/string-safe, with tests for each.

Agreed on the unbounded named-END search — noted as a follow-up, not blocking this PR.

Full suite passing (2400+ tests), ruff and mypy clean, pushed.

@satwikjambula

Copy link
Copy Markdown
Author

Hi @tirth8205 — this addresses all the feedback from both review rounds (the four original blockers, the mid-word AS/IS boundary issue, and the follow-up hardening pass), plus the branch is now rebased clean against current main with no conflicts. Full suite (2440+ tests), ruff, and mypy all pass. Would appreciate your review and, if it looks good, an approve/merge whenever you have a chance.

@tirth8205

Copy link
Copy Markdown
Owner

The correctness work from both rounds holds up, and the suite, ruff, and mypy are green on a merge with current main. One performance defect needs fixing before merge: in _extract_plsql_calls (parser.py:5166), the INSERT INTO filter runs re.search over body_text[:m.start()] for every call-site match, rescanning the whole preceding body each time, so call extraction is quadratic. Measured: 8k call sites in a 40KB .pls body takes 1.3s, 32k sites in 160KB takes 25s, and real package bodies in the hundreds of KB would stall a build for minutes. A bounded lookback of a few characters fixes it; the per-target line count at parser.py:5171 has the same shape and could use a running counter. Two smaller notes, non-blocking: _strip_plsql_noise resets string state per line, so a multi-line string literal containing 'END p;' truncates the span and drops later calls, and _PLSQL_IDENT is ASCII-only, so cafe-accented identifiers that main captured in full now truncate at the first non-ASCII char. Happy to merge once the quadratic is fixed.

_extract_plsql_calls checked for a preceding INSERT INTO by re-slicing
and re-searching body_text[:m.start()] on every call-site match, making
call extraction quadratic (25s+ on large files with many calls). Replace
it with a fixed-size lookback window, which keeps the check local (INTO
always appears immediately before its target) while making extraction
roughly linear again.
…l-support

# Conflicts:
#	tests/test_multilang.py
@satwikjambula

Copy link
Copy Markdown
Author

The correctness work from both rounds holds up, and the suite, ruff, and mypy are green on a merge with current main. One performance defect needs fixing before merge: in _extract_plsql_calls (parser.py:5166), the INSERT INTO filter runs re.search over body_text[:m.start()] for every call-site match, rescanning the whole preceding body each time, so call extraction is quadratic. Measured: 8k call sites in a 40KB .pls body takes 1.3s, 32k sites in 160KB takes 25s, and real package bodies in the hundreds of KB would stall a build for minutes. A bounded lookback of a few characters fixes it; the per-target line count at parser.py:5171 has the same shape and could use a running counter. Two smaller notes, non-blocking: _strip_plsql_noise resets string state per line, so a multi-line string literal containing 'END p;' truncates the span and drops later calls, and _PLSQL_IDENT is ASCII-only, so cafe-accented identifiers that main captured in full now truncate at the first non-ASCII char. Happy to merge once the quadratic is fixed.

Thanks for the detailed measurements @tirth8205 — confirmed the same quadratic behavior locally.

Root cause: _extract_plsql_calls (parser.py) checked for a preceding INSERT INTO by re-slicing and re-searching body_text[:m.start()] on every call-site match, so the whole preceding body got rescanned on each match — O(n²) overall.

Fix: replaced the full-prefix slice with a fixed-size lookback window (_INTO_LOOKBACK = 60 chars). INTO always appears immediately before its target, so a small bounded window is enough to catch it correctly (including across a couple of formatting/newline variations), without rescanning from the start of the body every time.

Verification (local):

Reproduced the quadratic scaling before the fix: 1,500 calls → 0.54s, 3,000 → 1.81s, 6,000 → 7.13s.
After the fix: 1,500 → 0.15s, 3,000 → 0.13s, 6,000 → 0.43s, 12,000 → 1.34s — roughly linear.
Added two regression tests: one asserting 4,000 calls complete in well under 5s (would fail immediately on a quadratic reintroduction), and one confirming the bounded lookback still correctly filters INSERT INTO across multi-line formatting.
Full suite, ruff, and mypy all pass.

Also merged current main into the branch and resolved the resulting conflicts in README.md and tests/test_multilang.py — both were purely positional (adjacent/same-line additions from unrelated PRs), not logical conflicts. Re-ran the full suite after the merge to confirm nothing broke.

Scope stayed limited to the reported blocker — I haven't touched the two non-blocking items you flagged (multi-line string literals in _strip_plsql_noise, ASCII-only identifier regex) unless you'd like those addressed in this PR too, happy to follow up.

@satwikjambula

Copy link
Copy Markdown
Author

Hi @tirth8205 — following up on this PR. All feedback from the reviews so far has been addressed:

The original 4 blockers (non-Oracle regression, forward-declaration phantom members, call-extraction noise, spec/body node collision)
The forward-declaration IS/AS word-boundary + comment-handling defect from the follow-up review
The quadratic performance regression in _extract_plsql_calls (bounded lookback window, verified linear up to 12k calls)
Merge conflicts against current main (resolved and re-verified)
Full test suite (2440+ tests), ruff, and mypy are all passing on the latest commit. This has been open for a while now — whenever you get a chance, could you take another look and let me know if it's good to merge, or if anything else needs changing?

Thanks for all the detailed review so far, it's helped a lot in getting this right.

@tirth8205

Copy link
Copy Markdown
Owner

Outside this sweep's mission of demonstrated coding-session token savings: this introduces a substantial Oracle PL/SQL dialect surface. There is also a supported-SQL regression: save CREATE PROCEDURE café AS SELECT 1; in names.sql and call CodeParser(repo_root).parse_file(path); this branch names the procedure caf, while main preserves café. That identifier loss needs correcting before the expansion is reconsidered.

@tirth8205
tirth8205 changed the base branch from main to staging September 15, 2026 13:12
satwikjambula and others added 2 commits September 15, 2026 08:59
_PLSQL_IDENT was restricted to [A-Za-z_][A-Za-z0-9_$#]*, so an
identifier with a non-ASCII letter (e.g. "café") got silently truncated
at the accented character, unlike the \w-based identifier matching used
for non-Oracle SQL elsewhere in the file. The leading character class is
now [^\W\d] instead of [A-Za-z_], keeping the special-cased $/# handling
for mid-identifier characters while accepting Unicode letters like the
rest of the file does.
@satwikjambula

Copy link
Copy Markdown
Author

Outside this sweep's mission of demonstrated coding-session token savings: this introduces a substantial Oracle PL/SQL dialect surface. There is also a supported-SQL regression: save CREATE PROCEDURE café AS SELECT 1; in names.sql and call CodeParser(repo_root).parse_file(path); this branch names the procedure caf, while main preserves café. That identifier loss needs correcting before the expansion is reconsidered.

Good catch — confirmed the regression with your exact repro.

Root cause: PLSQL_IDENT was [A-Za-z][A-Za-z0-9_$#]*, an ASCII-only identifier pattern, unlike the \w-based identifier matching used elsewhere in this file for non-Oracle SQL (\w is Unicode-aware by default in Python). So café got silently truncated to caf in the PL/SQL path while main correctly preserved it via \w.

Fix: changed the leading character class from [A-Za-z_] to [^\W\d] (word character that isn't a digit — i.e. Unicode letter or underscore), keeping the rest of the class ([\w$#]*) to preserve the existing $/# mid-identifier handling Oracle allows (e.g. log_event$v2).

Verification:

CREATE PROCEDURE café AS SELECT 1; now correctly produces a node named café, matching main's behavior.
Confirmed $/# handling is untouched — log_event$v2 still parses as a single identifier, not truncated at the $.
Added two regression tests: one for the accented-identifier case, one confirming $/# still can't start an identifier (matching Oracle's own rules).
Full suite (3000+ tests), ruff, and mypy all pass.

This addresses the identifier-loss regression you flagged as blocking further consideration of the PL/SQL expansion. Let me know if there's anything else you'd like adjusted.

@tirth8205

tirth8205 commented Sep 15, 2026 •

Copy link
Copy Markdown
Owner

This no longer merges into staging. Conflicts in CHANGELOG.md, README.md, code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md, docs/FEATURES.md, docs/USAGE.md.

git fetch origin && git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git push

When resolving:

  • Rebase or merge origin/staging into the PR branch and resolve conflicts in CHANGELOG.md, README.md, code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md, docs/FEATURES.md, docs/USAGE.md before re-assessment.

A large integration branch landed on staging today, which is why this drifted.

PRs now target staging, not main. Yours was retargeted already, so nothing to do there.

@tirth8205 tirth8205 added the needs-rebase Branch no longer merges into staging label Sep 15, 2026
…lsql-support

Resolves conflicts against the current staging branch in CHANGELOG.md,
README.md, code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md,
docs/FEATURES.md, and docs/USAGE.md — all positional/wording-refresh
conflicts from unrelated upstream changes, preserving the PL/SQL
mentions added by this branch alongside upstream's rewrites.
@satwikjambula

Copy link
Copy Markdown
Author

tirth8205 resolved conflicts

@tirth8205

Copy link
Copy Markdown
Owner

The conflicts are resolved properly. README stays at 31 feature rows with no duplicated rows, and CHANGELOG.md, LLM-OPTIMIZED-REFERENCE.md and USAGE.md each take staging's text plus your one line. docs/FEATURES.md came out identical to staging, which is the right result.

Gates on ea799b4 merged into staging 9c13f4c: 3381 passed, 9 skipped, 2 xpassed, ruff clean, mypy clean. The identifier regression is fixed: CREATE PROCEDURE café AS SELECT 1; now parses to Function 'café'.

So there is no technical objection left on this branch.

I am not queueing it for merge, and the reason is not a defect. Whether to take on an Oracle PL/SQL dialect surface at all is a scope call that has not been made, and that call belongs to the maintainer, not to this review pass. Nothing for you to fix while it is pending.

One thing to be aware of: only GitGuardian has run on ea799b4. The full CI matrix has not, so the numbers above come from running the three gate commands locally on the merge, not from CI.

Removing needs-rebase.

@tirth8205 tirth8205 removed the needs-rebase Branch no longer merges into staging label Sep 16, 2026
@tirth8205

Copy link
Copy Markdown
Owner

This no longer merges into staging. 2 files conflict and the branch is 144 commits behind:

  • CHANGELOG.md
  • README.md

All of it is prose, so taking staging's text and re-applying your lines on top should be the whole job.

git fetch origin
git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git push

I have not reviewed the change itself yet. That comes once it merges and the checks run against the merged state, since staging has moved a long way and the result is what matters.

@tirth8205

Copy link
Copy Markdown
Owner

Thank you for this PR. I tested 945b202 with small PL/SQL and T-SQL files. The 681 parser tests pass. But I found bugs I missed in my last review.

Before I can merge it, please fix these:

  1. String state resets on every line (code_review_graph/parser.py:5111). Example: msg VARCHAR2(100) := 'a, phantom() and b'; on three lines. This adds a false phantom call. A string with END demo; ends the procedure too early. Q-quotes like q'[it's phantom() here]' break the same way.
  2. A /* inside a string starts a comment (code_review_graph/parser.py:5104). This also breaks plain T-SQL files. Example: CREATE PROCEDURE a AS SELECT '/*' AS marker FROM users;, GO, CREATE PROCEDURE b AS SELECT * FROM orders;, GO, /* note */. staging finds a, b and both tables. This branch finds only a and no tables.
  3. The END name; search runs to the end of the file (code_review_graph/parser.py:5722). Take two package bodies, each with a member run. If the first run ends with a bare END;, it runs into the second package. It then gets that package's calls. Big files are also slow: 4,000 members took about 4 seconds. I called this a follow-up before, but please fix it now.

Please add a test for each case.

After you push the fix, I will test it again.

claude and others added 2 commits October 18, 2026 00:19
…ck-end search

_strip_plsql_noise reset its in-string flag per line and blanked /* */
comments over raw text in a separate first pass. A string spanning
multiple lines let an embedded "END name;" terminate the block early;
an Oracle q'[...]'/Q'<...>' quoted string (unescaped ' allowed inside)
broke the same way; and an unterminated /* inside one string literal
could pair with an unrelated */ inside a later string and blank out
everything in between, silently dropping statements that fell in that
span (this also regressed plain non-Oracle SQL files). Rewritten as one
pass over the whole text that tracks string and comment state together.

_plsql_block_end's named-END search also ran unbounded to end-of-file,
so a package body member with a bare END; could match a different,
later package body's same-named member instead of its own, and the
unbounded scan was quadratic on files with many members. It now takes
a limit, bounding a member's search to its own enclosing package body.
@satwikjambula

satwikjambula commented Oct 18, 2026 •

Copy link
Copy Markdown
Author

Thank you for this PR. I tested 945b202 with small PL/SQL and T-SQL files. The 681 parser tests pass. But I found bugs I missed in my last review.

Before I can merge it, please fix these:

  1. String state resets on every line (code_review_graph/parser.py:5111). Example: msg VARCHAR2(100) := 'a, phantom() and b'; on three lines. This adds a false phantom call. A string with END demo; ends the procedure too early. Q-quotes like q'[it's phantom() here]' break the same way.
  2. A /* inside a string starts a comment (code_review_graph/parser.py:5104). This also breaks plain T-SQL files. Example: CREATE PROCEDURE a AS SELECT '/*' AS marker FROM users;, GO, CREATE PROCEDURE b AS SELECT * FROM orders;, GO, /* note */. staging finds a, b and both tables. This branch finds only a and no tables.
  3. The END name; search runs to the end of the file (code_review_graph/parser.py:5722). Take two package bodies, each with a member run. If the first run ends with a bare END;, it runs into the second package. It then gets that package's calls. Big files are also slow: 4,000 members took about 4 seconds. I called this a follow-up before, but please fix it now.

Please add a test for each case.

After you push the fix, I will test it again.

Thanks tirth8205 for the detailed repro notes — confirmed all three locally and fixed.

Root cause (shared by all three string-state bugs): _strip_plsql_noise reset its in-string flag at the start of every line, and stripped /* */ comments over the raw text in a separate first pass before any string-awareness existed. That caused:

A string literal spanning multiple lines let an embedded END name;-shaped substring terminate the block early.
Oracle’s q'[...]'/Q'<...>' quote operator (which allows unescaped ' inside the string) confused the plain '...' state machine the same way.
An unterminated /* inside one string literal could pair up with an unrelated */ inside a later string, blanking out everything in between — including a whole CREATE PROCEDURE statement that happened to fall in that span. This is exactly what broke the T-SQL case: the table’s CREATE TABLE kept getting swallowed by a runaway fake comment that started inside an unrelated string earlier in the file.

Fix: rewrote _strip_plsql_noise as a single pass over the whole file that tracks string and comment state together (plain strings, Q-quoted strings, -- and /* */ comments), instead of per-line state + a separate raw-text comment pass.

Second bug — cross-package-body bleed / slowness: _plsql_block_end’s named-END search ran unbounded to end-of-file, so a member using a bare END; could skip past its own package body entirely and match a different, later package body’s same-named member’s END name;. The same unbounded scan was also quadratic over a file with many members (the ~4s for 4,000 members you measured). Added a limit parameter; a member’s search is now bounded to its own enclosing package body’s end.

Tests added (one per case you flagged):

  • multi-line string truncating a block early
  • Q-quoted string with an embedded '
  • comment-in-string swallowing a subsequent CREATE PROCEDURE/CREATE TABLE
  • cross-package-body END bleed between two same-named members
  • a 2,000-member package body to confirm the bounded search stays fast

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants