Repository navigation
fix: give every parsed object a node spanning its source - #2870
knutwannheden wants to merge 7 commits into
Conversation
prattArithRest builds operator expressions in plain Java, outside any production, so JJTree never opened a node for them and `CURRENT_DATE - 7` or `a || b` had no source range. Each expression the loop builds is now linked to a node from the token its operand chain starts at to the last token consumed. The node stays out of the tree, so the operands' nodes keep their place.
|
Converting this to a draft while I extend it to the other expressions that still come out of the parser without an AST node. Going through the test corpus turned up nested |
The statement classes that implemented Statement alone now extend ASTNodeAccessImpl, as Select and Import already did, so a parsed statement can hold the node that records its source range. Nothing links them yet.
Objects no grammar rule opens a node for, such as a column's table qualifier, a WHEN clause, nested AND/OR, IS NULL, BETWEEN, parameter lists, data types, ORDER BY elements and statements, are linked to a RangeNode: a node outside the parse tree that records the object's first and last token. getASTRoot() returns the same tree, and no tree node gains or changes its value. Ranges that missed text their object prints now cover it: a statement's WITH, Oracle PRIOR and (+), LIMIT ... BY, sequence options, IF ... ELSE and merged adjacent literals. A string literal no longer takes the node of the primary expression around it. Comments get positions, so an Oracle hint has a range too. getParent() returns null instead of throwing once it runs out of tree. ASTNodeCoverageTest parses the test resources' SQL and requires every ASTNodeAccess object to have a node of at least one character, inside its holder's range, never partly overlapping another, holding no parser, and, for expressions and statements, covering text that parses back to the same object.
knutwannheden
changed the title
Statements that never pass through SingleStatement get their node where they are built: data-modifying CTEs, statements inside an Oracle block, CREATE SCHEMA elements, CREATE TABLE ... AS EXECUTE and the GRANT or REVOKE of ALTER DEFAULT PRIVILEGES. An IF statement's range takes in the semicolon it prints. A RangeNode that widens a tree node answers its children, id and dump too, so a WITH statement's node still shows the tree beneath it. Comments are positioned in one backwards pass over the recorded match begins, and comments before the end of input are positioned once parsing ends, since the end-of-file token gets no token action. Values in a primary expression, string literals, expression lists and row patterns get a RangeNode only once something wraps them, so a production's own tree node is the only node most of them get. On a 20,000-row INSERT ... VALUES this cuts the added allocation from 12.9% to 3.2%. The coverage test parses the Oracle tests in the Oracle dialect, checks that getParent() encloses each range, and runs its checks over the statements above, which the test corpus does not contain.
|
Please fix Codacy issues. |
`visit` and `checkRange` exceeded PMD's NPath threshold of 200, so the container walk and the parent check move into methods of their own. `isHeld` compares with `==` on purpose: a getter that answers a new object on every call computes its child rather than holding it.
|
Thanks! The Codacy findings are fixed in e8de6e9. All four were in the new I ran
This branch parses |
|
Good Job, give me a few minutes for proof reading everything. |
`Condition()` created a deque for the tokens of a leading `NOT` on every call, though few conditions have one. Each empty `ArrayDeque` allocates its 16-slot array up front, so parsing `performance.sql` allocated 585 KB more per parse. The deque is now created at the first `NOT`.
|
Most of that came from my own change: a deque
What remains is the range nodes themselves.
The branch now averages 0.7% slower, less than master moved between its own two runs (2.3%). Deriving ranges from operands would remove only a small part of what is left, so I'd leave that for a follow-up. The same profile shows a few allocations on master worth trimming, such as the |
Only the generated parser creates a `RangeNode`, and callers see it as the `Node` that `getASTNode()` returns. Keeping the class package-private leaves its design free to change before a release.
|
Looking past this PR: for the next major release, I think it would be cleaner to stop exposing the parser's internals through the AST.
What callers need is the source range. |
|
Sorry for asking a stupid question: why not implement for |
Most objects JSqlParser returns cannot be mapped back to their source text. Across the 1,513 statements in the test suite's SQL, 6,575 objects that implement
ASTNodeAccessreturn null fromgetASTNode(). Among them are every column's table qualifier, everyWHEN ... THEN, nestedAND/ORand inner arithmetic. Some existing ranges miss part of their text:ASTNodeAccessobjects without a nodeOn master, statements,
OrderByElementandColDataTypedo not implementASTNodeAccess, so they are not counted.What changes
RangeNode. Objects that no grammar rule gives a node are linked to aRangeNode, a node outside the parse tree that records the object's first and last token. These are objects built in the Pratt operator loops, in productions without a#node, or owned by the model, such as aPARTITION BYclause.getASTRoot()returns the same tree, and no tree node changes its value or range.WITH, an OraclePRIOR, a column's(+),LIMIT ... BY, sequence options,IF ... ELSEand merged adjacent literals. The object moves to aRangeNodeover its full text, which answers its tree node's parent, children and id, sogetParent()and tree walks from the object's node are unchanged. A string literal no longer takes the node of the primary expression around it, as in'a'::text.SimpleCharStreamnow records where each match began.CommonTokenActionthen gives the comments before each token their positions, and an Oracle hint gets a range.OrderByElement,ColDataType, and the 118 statement classes that implemented onlyStatementnow extendASTNodeAccessImpl.SingleStatement(), blocks and unsupported statements link them. The statement sweep is its own commit.How it is checked
ASTNodeCoverageTestparses the SQL files undersrc/test/resourceswith error recovery, the Oracle tests in the Oracle dialect, and walks every object through its getters. EachASTNodeAccessobject must have a node that:getParent()returns;The same checks run over statements the corpus lacks, such as data-modifying CTEs and Oracle blocks. Another test pins ranges the checks cannot judge, such as
WHEN, data types,PARTITION BY,(+),LIMIT BYand hints. Each check fails when one of the fixes is undone.Compatibility and cost
getParent()returns null when it runs out of tree, instead of throwing. ARangeNodefor an object with no tree node has no parent.prattArithRestandprattExpressionResttake the token their operand chain starts at.INSERT ... VALUESallocates 3.2% more and retains the same, and its parse time shows no difference: 3 pairs faster, 2 slower, 1 tied.Not covered
app@localhost, has no token of its own.ASTNodeAccess, such asAlias,ColumnDefinition,WithItem,GroupByElementandUpdateSet, still have no range.