gregfelice commented on PR #2360:
URL: https://github.com/apache/age/pull/2360#issuecomment-4772392371
@jrgemignani — closing out the Opus 4.7 review items.
**1. `-Werror` scope** — done (`c6270db0`): kept `-Werror`, downgraded only
`-Wno-error=conflicts-{sr,rr}`. Any other Bison warning still fails the build.
**2. `%expect` budget** — one correction, because the suggested fix can't
compile, then how I solved the real goal. Bison treats `%expect` / `%expect-rr`
as **exact-match, not a ceiling**: `%expect 10` against a real count of 7 fails
with *"expected 10 shift/reduce conflicts, found 7."* So widening to a generous
10/5 reds the build immediately on our Bison (3.8.2 → 7 s/r, 3 r/r).
The underlying goal — don't let distro Bison drift silently break CI, but
still catch genuinely new conflicts — is right, so I pinned the toolchain
instead of loosening the alarm (just pushed):
- `installcheck.yaml`: `runs-on: ubuntu-latest` → `ubuntu-24.04`.
`ubuntu-latest` floating to a newer release (and a newer Bison) was the actual
drift source; freezing the image freezes Bison at 3.8.x.
- Added a "Verify Bison version" step that fails *loudly with a pointer to
`cypher_gram.y`* if Bison ever leaves 3.8.x — so the next person gets "re-run
bison, update %expect, bump the runner together," not a cryptic count mismatch
deep in the build.
Net: `%expect 7 / %expect-rr 3` stays exact (tightest alarm for a real new
conflict), reproducibility comes from pinning the variable. Same posture as
PostgreSQL's `gram.y`, which keeps an exact `%expect` and bumps it on
intentional changes.
**3. Parse-time microbench (GLR vs LALR)** — ran it. Disposable PG18
container, identical AGE build per arm except the grammar, `pgbench -M simple`
(simple protocol = full re-parse every txn — the regime GLR taxes), `-c4`,
1000-node graph, Bison 3.8.2.
| query | LALR tps | GLR tps | Δ |
|---|---|---|---|
| `RETURN 1` (parse floor) | 123,849 | 123,815 | −0.03% |
| `MATCH (a:Person {id:$1}) RETURN a` | 25,170 | 25,463 | +1.2% |
| `RETURN (a)-[:KNOWS]->(:Person)` (pattern expr) | parse error (LALR) |
17,156 | GLR-only |
**No measurable parse-time regression** — the parse floor is identical and
the point lookup is within run-to-run noise (GLR came out marginally ahead),
well under the >5% bar. The pattern-expression query is GLR-only (new syntax);
its lower tps is EXISTS-sublink *execution* cost, not parse overhead.
**4. Follow-up test coverage** — already added: pattern expressions in list
literal, map literal, `count()`, `OPTIONAL MATCH … WHERE` (null-preserving),
and mixed `EXISTS((..)) AND (..)`.
**Still open from the review — want me to fold these in, or take as a fast
follow?**
- One coverage gap remains: `RETURN (a:Label)` (single-node labeled pattern
as a boolean). Quick to add.
- PR-body note that the `YYRHSLOC` change is a required GLR correctness fix,
not cosmetic.
- PR-body note that pattern expressions in projection are
EXISTS-sublink-per-row (O(rows × subquery)), distinct from WHERE where the
planner may pull it up.
--
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]
