Repository navigation
Conversation
|
Thanks for the pull request, @Yi-111-a. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; no formal proof of the changed code.
Graphify review — findings
Accepts Oracle's optional EDITIONABLE/NONEDITIONABLE keyword in the SQL extractor's routine and trigger recovery patterns, so CREATE OR REPLACE [NON]EDITIONABLE PROCEDURE|FUNCTION|TRIGGER (the default DBMS_METADATA.GET_DDL output) yields the same routine, trigger, and subject-table edges as plain DDL. Previously such files recovered no routine at all (#5167). The keyword is only matched between CREATE [OR REPLACE] and the object kind, so a table or column named editionable still doesn't mint a phantom routine; new tests cover each kind, mixed case, and that negative case.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 3165 functions depend on the 36 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_sql()— 46 callers, 13 callees
Verification — 3165 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1218 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
7 of 560 test file(s) selected (1%) via static blast radius.
tests/test_extract.py— impacttests/test_multilang.py— impacttests/test_pg_introspect.py— impacttests/test_python_member_call_language_gate.py— impacttests/test_sql_go_batches.py— impacttests/test_sql_oracle_editionable.py— impact, changed-testtests/test_sql_tsql_procedures.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
· 1 more finding(s) on lines outside this diff (see the check run).
What does this PR do?
Fixes #5167. Oracle's default DDL export (
DBMS_METADATA.GET_DDLand most IDE exports) writesCREATE OR REPLACE EDITIONABLE PROCEDURE|FUNCTION|TRIGGER(orNONEDITIONABLE). Those statements land in anERRORnode, and the recovery regexes_ROUTINE_RECOVERY_RXand_TRIGGER_RECOVERY_RXonly allowedCREATE [OR REPLACE|ALTER]directly followed by the object kind, so the routine or trigger was never recovered and the file came back with only its file node.Both regexes now accept an optional
(?:NON)?EDITIONABLE\s+betweenCREATE [OR REPLACE]and the object kind, which is the change proposed in the issue.Type of change
Verification & Invariants
Invariant: adding
EDITIONABLE/NONEDITIONABLEto a routine or trigger DDL does not change the extracted nodes or edges.On
v8with tree-sitter-sql 0.3.13,CREATE OR REPLACE NONEDITIONABLE FUNCTION hr.f ...andCREATE OR REPLACE EDITIONABLE TRIGGER hr.trg_emp ... ON hr.emp ...returned only the file node. With this change they producehr.f()(with itsreads_from hr_emp) andhr.trg_emp(withtriggers hr_emp), identical to the same files without the keyword.tests/test_sql_oracle_editionable.pychecks that procedure, function and trigger DDL withEDITIONABLE,NONEDITIONABLEand lower-caseeditionableextract exactly the same graph as the plain form, that the trigger keeps its subject table, and that a table literally namededitionabledoes not turn into a routine. The function and trigger cases fail without the fix.Limitation: with 0.3.13 the
EDITIONABLE PROCEDURE ... IScase from the issue already recovered the procedure onv8for me, so this mainly changes functions and triggers here; the test still pins the procedure case. The missingwrites_to hr_emp_copyforINSERT INTO ... SELECT *inside the body happens with or without the keyword and is left alone. The Firebirdfb_proc_or_triggerpath is untouched sinceEDITIONABLEis Oracle-only. This changes per-file extractor output, so it may want to ride along with the next cache schema bump.How was this tested?