gregfelice commented on PR #2360:
URL: https://github.com/apache/age/pull/2360#issuecomment-4444676978
@jrgemignani — thanks for the careful review. Addressed all three blocking
asks in `553e9761`, with one factual correction to the underlying reasoning
that I want to surface explicitly:
**1. Makefile `-Werror` scope** — kept `-Werror`, downgraded only the two
conflict categories via `-Wno-error=conflicts-sr -Wno-error=conflicts-rr`. pgxs
auto-adds `-Wno-deprecated` so the existing `%name-prefix=` / `%pure-parser`
directives still build clean. Any other Bison warning (unused rules, undeclared
types, etc.) now fails the build again — same coverage as master, minus the two
known-noise categories.
**2. `%expect` / `%expect-rr` budget** — added `%expect 7` and `%expect-rr
3` matching the Bison 3.8.2 totals. **Caveat:** Bison treats `%expect` as
**exact-match**, not as a ceiling, so the "wider budget to absorb Bison drift"
framing in the review doesn't actually work — `%expect 12` on a 7-conflict
grammar fails the build with `found 7, expected 12` (verified empirically).
Exact pinning is actually a stronger signal than a generous bound: it catches
both new conflicts AND silent disappearance of expected ones (either is a
surprise worth auditing). The grammar comment documents that future Bison
versions reporting different counts should bump these numbers explicitly with a
version note in the commit message.
**3. Parse-time microbench — Phase 3.5 release-build perf preflight** —
built a user-owned PG18 release cluster (`-O2`, no cassert) and ran a pgbench
equivalent of the workload-mapping-prescribed smoke (W01 + W04 for parser-only
changes), 5 iterations × 15 s × 1 client per side:
| | LALR (master)
mean | GLR (this PR) mean | delta |
|----------------------------------------------------------|-------------------:|-------------------:|------:|
| **W04-style**: `MATCH (a:L {p: 42}) RETURN a` (1000-node graph) | 4944 TPS
(4904–4976, ±1.5%) | 4979 TPS (4961–4999, ±0.8%) | +0.7% |
| **W01-style**: `CREATE (:L {p: 99})` (in rolled-back tx) | 11064
TPS (10749–11353, ±5.6%) | 11066 TPS (10920–11176, ±2.3%) | +0.02% |
Both deltas well below the 5% gate, and W01's LALR-side range (5.6%) is
itself wider than the LALR-vs-GLR delta. No measurable parse-time regression on
representative read-only or write Cypher.
Cassert installcheck: 34/34 AGE tests green (pgvector skipped — `vector`
extension not installed in my local cassert PG; unrelated to this PR).
I also updated the PR description to (a) note the `YYRHSLOC(rhs, 1)`
replacement is a GLR-stack correctness fix, not stylistic, and (b) reflect the
restored `-Werror` + `%expect 7/3` approach.
**Non-blocking follow-ups from the review** — your call on bundling:
- Test coverage for `RETURN (a:Label)`, list/map literals, function args,
`OPTIONAL MATCH WHERE` — happy to fold into this PR's `pattern_expression`
regression suite as a second commit if you want them here, otherwise I'll open
a follow-up.
- Doc note on EXISTS-sublink-per-row cost in projection contexts — better as
a standalone docs commit so it's discoverable from the Cypher reference, not
just this PR's history.
--
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]
