Incremental Update: Ashutosh Bapat's Review and v3 Patch Restructuring
New Reviewer: Ashutosh Bapat (2026-05-14)
Ashutosh Bapat, a key contributor to SQL/PGQ infrastructure, weighed in with a cautious endorsement tempered by forward-compatibility concerns.
Strategic concern — interaction with future features: Ashutosh raises the question of whether the "fail early" pruning implementation will complicate or simplify upcoming SQL/PGQ features: variable-length element patterns and embedded path patterns. These features will change the enumeration structure itself, and a pruning strategy baked into the current DFS shape might need rework. He frames the trade-off explicitly: delaying the optimization until those features land is the "safest strategy," but acknowledges it should be incorporated "sooner or later."
Practical skepticism on urgency: Ashutosh notes the pathological examples demonstrated so far are "mostly artificial, not practical" — challenging the implicit assumption that the pruning is needed now. He suggests bugs in other areas may be higher priority.
Technical suggestion — unify feasibility logic with join-tree construction: Rather than maintaining duplicate direction-checking logic in both generate_query_for_graph_path (post-hoc) and the new graph_path_edge_is_feasible (early), Ashutosh proposes starting to construct the Join tree during path enumeration so edge direction feasibility and edge connection quals are produced in a single pass. This would eliminate the consistency-maintenance burden between the two code paths, though he acknowledges the need to discard intermediate objects for pruned branches.
Request to separate CHECK_FOR_INTERRUPTS: Ashutosh asks that CHECK_FOR_INTERRUPTS be split into its own independently-committable patch, arguing it has standalone value regardless of the optimization's fate. He also asks for direct evidence of time spent inside the recursive function (not just total query time) to justify its placement.
Cosmetic feedback:
- Move the
list_length(graph_path) == 1early-return higher in the function for readability. rev_feasibleneeds more explanatory comments.
Junwang's v3 Response (2026-05-14)
Junwang addresses all points and restructures the patch series into four parts:
| Patch | Content | Committability |
|---|---|---|
| 0001 | CHECK_FOR_INTERRUPTS() in DFS recursion (standalone) |
Independent, committable now |
| 0002 | Temporary timing instrumentation (demonstration only) | Not for commit |
| 0003 | Core early-pruning logic with updated comments | Dependent on design discussion |
| 0004 | Regression test for N^K enumeration + ASCII fix | Accompanies 0003 |
Timing evidence provided: Using 0002's instrumentation on the g5 benchmark query, Junwang shows that of 5577 ms total query time, 4648 ms (83%) is spent inside generate_queries_for_path_pattern_recurse. This directly answers Ashutosh's request and strongly justifies CHECK_FOR_INTERRUPTS placement inside the function.
Cosmetic fixes applied: Early-return moved up; additional comments on rev_feasible added.
Shift in Patch Strategy
The series has evolved from a monolithic optimization patch to a layered commit strategy where the safety-net (CHECK_FOR_INTERRUPTS) can land independently regardless of the pruning optimization's timeline. This directly accommodates Ashutosh's concern that the optimization may need to wait for variable-length pattern design work.
Outstanding Design Questions (New)
- Forward compatibility with variable-length patterns: No concrete analysis yet of how pruning interacts with future
{m,n}quantifier support. This is flagged but unresolved. - Feasibility logic duplication: The suggestion to unify early pruning with join-tree construction is architecturally interesting but would be a much larger refactor — no consensus on whether to pursue it now or leave it for future work.
- Whether the optimization lands in v18 or is deferred is now explicitly on the table as a process question.