jrgemignani commented on PR #2360:
URL: https://github.com/apache/age/pull/2360#issuecomment-4442461982
@gregfelice
From Opus 4.7 -
# Deep review: PR #2360 — Pattern expressions as boolean expressions
I read the PR description and the actual diff (`cypher_gram.y`, Makefile,
regression test). Status: 3 commits, force-pushed onto current master
(`84e29542`), Copilot's 3+2+0 rounds resolved, `jrgemignani` approved. Here's
my analysis.
## What it does (correctly)
The change is fundamentally sound:
1. **Switches `cypher_gram.y` to `%glr-parser`.** Right call — the `(a)`
ambiguity (parenthesized expr vs. node pattern) genuinely is unresolvable at
one-token lookahead. LALR(1) can't see far enough to know whether `(`
introduces `(1+2)` or `(a)-[:R]->(b)`. GLR is the textbook tool.
2. **Adds `anonymous_path %dprec 1` to `expr_atom`**, alongside the existing
`'(' expr ')' %dprec 2`. Bare `(a)` tied to both productions — `%dprec 2 >
%dprec 1` correctly disambiguates to expression-variable.
3. **Refactors EXISTS-wrapping into `make_exists_pattern_sublink()`** — and
re-uses it for both the existing `EXISTS(pattern)` production and the new
bare-pattern production. The refactor is byte-identical to what the inline code
did (same `cypher_sub_pattern`, `CSP_EXISTS`, `EXISTS_SUBLINK`,
`node_to_agtype(..., "boolean", ...)`). Zero semantic change for the existing
EXISTS path. ✓
4. **`YYRHSLOC(rhs, 1)` macro fix** — silently correct and necessary. GLR
has a different rhs stack layout than LALR(1); `(rhs)[1]` indexes the wrong
slot under GLR. This isn't mentioned in the description but it's a real
correctness fix the PR depends on.
5. **Tests cover meaningful surface area** — WHERE, NOT, AND/OR, RETURN
(alias + positional), mixed projections, CASE WHEN, SET, WITH pipeline,
backward-compat with `EXISTS(...)`. Good coverage of the new contexts.
## Concerns worth raising
### 1. Removing `-Werror` is heavier than needed
The Makefile change is:
```diff
- src/backend/parser/cypher_gram.c: BISONFLAGS += --defines=... -Werror
+ src/backend/parser/cypher_gram.c: BISONFLAGS += --defines=...
-Wno-conflicts-sr -Wno-conflicts-rr
```
This drops `-Werror` entirely, not just the two conflict categories. Any
*other* Bison warning (deprecated `%name-prefix=`, undeclared types, unused
rules, etc.) will silently be ignored going forward.
**Suggested:** keep `-Werror`, add only the two no-error opts:
```
BISONFLAGS += --defines=... -Werror -Wno-error=conflicts-sr
-Wno-error=conflicts-rr
```
Bison 3.4+ supports `-Wno-error=<category>`. Keeps the safety net on
everything else.
### 2. No `%expect` budget at all is fragile
The description's argument — "exact counts can drift across Bison versions"
— is correct. But going to "any number of conflicts is fine" loses the alarm
bell that catches *new, unintentional* conflicts a future grammar change might
introduce. A new rule that adds 12 more conflicts would silently slip in.
**Suggested:** `%expect 7 / %expect-rr 3` with a code comment "expected to
drift on Bison upgrades; update as needed." This is the standard PostgreSQL
approach in `gram.y` (which has `%expect 0` and updates when needed). The point
of `%expect` is not to pin a version, it's to fail loudly when *unexpected*
conflicts appear. If the worry is distro drift, the right answer is a wider
`%expect` value (e.g., `%expect 12 %expect-rr 5`) — generous enough to absorb
Bison-version churn, tight enough to flag a new grammar bug.
### 3. GLR runtime cost on the parse hot path
GLR forks parse stacks at every conflict point. On ambiguous prefixes it
does several full reductions before discarding losers. AGE parses Cypher on
every single `cypher()` call, often inside tight per-row contexts (UDF
arguments, prepared statements re-planned, etc.). I don't see any parse-time
benchmark in the PR.
Concretely: a `MATCH (a)` query enters the `(` ambiguity at character 7, and
GLR will explore both `'(' expr ')'` and `anonymous_path` until it sees the
closing `)` and what follows. For short read-only queries, parser time is
non-trivial relative to plan+execute.
**Recommended diligence before merge:**
- Microbench: `pgbench -f script.sql -T 30 -c 4` running 1000× of typical
Cypher query (`MATCH (a:Person {id: $1}) RETURN a`) on LALR build vs. GLR
build. Look for backend CPU% in parse.
- Spot-check `EXPLAIN (ANALYZE, BUFFERS) SELECT cypher(...)` planning time,
before/after.
If parse time regresses meaningfully (say >5%) on simple queries, this is
worth knowing and documenting, even if it's accepted as the cost of the
feature. If it's noise (<1%), it's a non-issue.
### 4. Pattern expression semantics — coverage gaps
The grammar change makes `anonymous_path` legal anywhere an `expr_atom` is.
That opens the door wider than the tests probe:
- **Single-node labeled patterns in expression position**: `RETURN
(a:Person)` is, in openCypher, the pattern "does `a` have label `Person`?" — a
boolean. Under this PR, `(a:Person)` would be parsed by GLR — does it pick the
`(expr)` branch (which would fail because `a:Person` isn't an expression) or
the `anonymous_path` branch (which would succeed)? Per `%dprec`, both fork; the
failing fork is dropped; we get the pattern interpretation. The test suite
doesn't include this case. Worth adding.
- **In list/map literals**: `RETURN [(a)-[:R]->(b), (a)-[:S]->(c)]` — list
of booleans. `RETURN {flag: (a)-[:R]->(b)}` — map containing a boolean. The
grammar now allows these. Untested.
- **In function arguments**: `RETURN count((a)-[:R]->(b))`. Should work
(boolean is countable). Untested.
- **In OPTIONAL MATCH WHERE**: `OPTIONAL MATCH (a) WHERE (a)-[:R]->(:L)`.
Untested.
- **Nested EXISTS**: `WHERE EXISTS((a)-[:R]->(b)) AND (a)-[:S]->(c)`. The
PR's "mixed" test doesn't combine the two syntaxes in one predicate. Untested.
None of these is a blocker — they're follow-up coverage. But the PR
description claims "matching what openCypher defines and what Neo4j users
expect," and these are exactly the cases Neo4j users will try.
### 5. Performance of the resulting plan
Bare pattern → `EXISTS` sublink → planner builds a correlated subquery. In a
`RETURN a.name, (a)-[:KNOWS]->(:Person) AS knows` over a million rows, the
planner has to either:
- (a) Evaluate the EXISTS sublink per row (O(N) sublink evals), or
- (b) Convert it to a semi-join (PG's `pull_up_sublinks`).
For Cypher, the pattern is a SRF (cypher's own scan), not a SQL subquery the
planner can semi-join in the usual way. So path (a) is what will happen, and on
large data it will be slow.
That's the *correct* semantics. But it's worth documenting in the PR or a
follow-up that pattern-expressions in projection contexts are O(rows × subquery
cost), distinct from pattern-expressions in `WHERE` (where the planner *might*
pull it up). Tests use 4 rows — invisible. Real workloads would notice.
### 6. The `expr_func_subexpr: EXISTS '(' anonymous_path ')'` production
stays
It's now redundant with `anonymous_path %dprec 1` in `expr_atom`. Both
produce the same `make_exists_pattern_sublink(...)` output. The grammar will
accept both spellings. Keeping the explicit form is fine for backward compat
(users may have written `WHERE EXISTS((...))` for years), but it does add to
the conflict count. Could be noted in the description as "intentionally
redundant for backward compatibility."
### 7. `var_name_opt` `%dprec 1` on both alternatives
```
var_name_opt:
/* empty */ %dprec 1
{ $$ = NULL; }
| var_name %dprec 1
;
```
Equal priorities on both alternatives is a no-op for `%dprec`. Bison will
treat this as "no preference" and discard the failing fork. That's fine, but
the annotation is misleading — the priorities don't disambiguate, they're
inert. Either remove them or set unequal values. Minor.
## Verdict
**Direction: correct.** GLR is the right tool, the refactor preserves
semantics exactly, the test coverage spans the new contexts, and Copilot's
review rounds caught the small stuff.
**Items I'd want addressed before merge:**
1. Keep `-Werror`, scope down to `-Wno-error=conflicts-{sr,rr}` rather than
nuking `-Werror`.
2. Re-introduce some `%expect` budget (generous, e.g. `%expect 10 %expect-rr
5`) so future grammar changes that add unintended conflicts get flagged.
3. Quick parse-time microbench (LALR vs. GLR) on a couple of representative
read-only Cypher queries. Even a one-line "no measurable regression on `MATCH
(a:L {p: $1}) RETURN a`" in the PR description would satisfy.
**Items I'd address as a follow-up (not blocking):**
4. Test coverage gaps: `RETURN (a:Label)`, pattern in list/map literals,
pattern in function args, OPTIONAL MATCH WHERE.
5. Document the EXISTS-sublink-per-row cost in projection contexts so users
aren't surprised on large data.
6. Note the `YYRHSLOC` fix in the PR body — it's not just a stylistic
change, it's required for GLR correctness.
**Items that are fine as-is:**
7. The `make_exists_pattern_sublink` refactor (semantics preserved
byte-for-byte).
8. The conflict explanation block comment (genuinely helpful for future
grammar maintainers).
9. The decision to broaden scope from "WHERE only" to "all expression
contexts" — Copilot was right, the grammar change inherently does this and a
WHERE-only restriction would have required a parallel grammar branch.
Overall, this is a high-quality feature PR that solves a real
openCypher-compat gap, and the maintainer approval is well-earned. The Makefile
`-Werror` removal and absent `%expect` budget are the two items I'd push back
on; everything else is polish.
--
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]
