rzo1 commented on PR #144:
URL: https://github.com/apache/openjpa/pull/144#issuecomment-5323990530
> Reading from file
/home/runner/work/openjpa/openjpa/openjpa-kernel/target/javacc-1785507996706/node/JPQL.jj
. . .
> Warning: Choice conflict in (...)* construct at line 1053, column 17.
> Expansion nested within construct and expansion following
construct
> have common prefixes, one of which is: "+"
> Consider using a lookahead of 2 or more for nested expansion.
> Warning: Choice conflict in (...)* construct at line 1062, column 17.
> Expansion nested within construct and expansion following
construct
> have common prefixes, one of which is: "*"
> Consider using a lookahead of 2 or more for nested expansion.
> Warning: Choice conflict involving two expansions at
> line 1533, column 12 and line 1534, column 12 respectively.
> A common prefix is: "AVG" "("
> Consider using a lookahead of 3 or more for earlier expansion.
> Warning: Choice conflict involving two expansions at
> line 1533, column 34 and line 1534, column 12 respectively.
> A common prefix is: <IDENTIFIER>
> Consider using a lookahead of 2 for earlier expansion.
I let Claude having a adversial review on this one. I would follow its
suggestions and move that in a separate Jira to avoid bloating this PR ;-)
This is the output:
Looked into the four JavaCC warnings. Short version: two are a real,
~20-year-old
correctness bug worth a separate JIRA; two are new on this branch but
semantically identical.
First, a mapping note: jjtree preserves the original `.jjt` token positions
when it writes
`JPQL.jj`, so those line/column numbers are already coordinates in
`JPQL.jjt`, not in the
generated file. I reproduced all four verbatim (identical lines *and*
columns) with the same
`javacc-5.0.jar` the build uses.
**Warnings 1 & 2 — `"+"` at 1053:17, `"*"` at 1062:17 - real bug,
pre-existing.**
`arithmetic_expression()` / `arithmetic_term()` eliminated left recursion by
recursing
*right* into themselves, so all binary arithmetic parses right-associative.
Precedence is
fine; associativity is not. Confirmed by dumping trees from the real parser:
| expression | tree today |
|---|---|
| `10 - 3 - 2` | `SUBTRACT(10, SUBTRACT(3,2))` = 9, not 5 |
| `x.a / x.b / x.c` | `DIVIDE(a, DIVIDE(b,c))` |
| `x.a - x.b + x.c` | `SUBTRACT(a, ADD(b,c))` |
Nothing downstream repairs it: `DBDictionary.mathFunction` (:3386-3406)
wraps every binary
op in its own parens, so the wrong grouping goes into the SQL verbatim.
`WHERE x.total -
x.paid - x.refunded > 0` is wrong today.
Pre-existing, not from this branch: the block i and
`git log -S` bottoms out at 1fede626e (2006, original code donation). Only
the line numbers
moved (974→1053) because ~79 lines were added a
Fix is to recurse into the next-tighter product
```diff
- ((<PLUS> arithmetic_expression() #ADD(2))
- | (<MINUS> arithmetic_expression(
+ ((<PLUS> arithmetic_term() #ADD(2))
+ | (<MINUS> arithmetic_term() #SUB
(and arithmetic_term → arithmetic_factor likewiarnings drop
4→2, trees become left-leaning, precedence preserved. I parsed a 1564-query
corpus harvested
from the test sources under both parsers: 0 acc exactly 2
tree-shape differences, both TestJPQLScalarExpressions.java:121,130
(SUM(c.age) - MIN(c.userid) + MAX(c.userid)), wo won't catch
the change.
Note the LOOKAHEAD(2) that JavaCC suggests is the wrong fix — I verified it
produces
byte-identical (still right-associative) trees.ithout fixing
anything.
Since this changes emitted SQL for unparenthesised chained arithmetic, I'd
file it as its
own JIRA with a release note and tree-shape regrser currently has
no arithmetic associativity assertion at all.
Warnings 3 & 4 — "AVG" "(" / <IDENTIFIER> at 1533/1534 — new here, but inert.
In orderby_item(), alternatives 4-5 (orderby_extension(),
identification_variable())
carry no LOOKAHEAD, so JavaCC resolves by firstuates
LOOKAHEAD(scalar_expression()) on alternative 6 — making that alternative
dead for
AVG/MIN/MAX/SUM/COUNT and for bare identifiers.
Introduced jointly: f90549c15 (NULLS FIRST/LASTion() alternative
first, where its lookahead suppressed the warnings; a5d724993 (ORDER BY
alias regression)
moved it below the two bare alternatives, which
But the affected queries fail on master too — O is a
ParseException there as well — so this is an unrealised extension, not a
regression, and
a5d724993 was a genuine fix that shouldn't be r under-report the
real shape of the gap: ORDER BY a.balance * 2 and ORDER BY -a.balance fail
too, silently,
via the lookahead-ful alternatives.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
