chore: release 3.2.0 — parenthesize operator operands, add Expr::Paren#13
Merged
Conversation
Renderers emitted no grouping, so an operand that was itself an operator
expression got re-associated by the engine's precedence rules — two different
ASTs rendered to the same SQL.
Cast(JsonPathText(data, 'age'), "bigint") produced `data->>'age'::bigint`,
which PostgreSQL reads as `data ->> ('age'::bigint)` and rejects. Worse,
Binary(Binary(1, +, 2), *, 3) produced `1 + 2 * 3` and silently evaluated to 7
instead of 9, with no error from the database.
Operands of Binary, Unary, Cast, Collate, JsonPathText and of both sides of a
comparison are now bracketed when they are themselves Binary, Unary, Collate,
JsonPathText or Window. Bracketing is structural rather than driven by a
precedence table, because precedence is dialect-specific: SQLite binds || tighter
than *, PostgreSQL binds it looser than +.
Raw and Custom are never bracketed automatically — their contents are opaque and
need not be an expression at all. The new Expr::Paren variant groups them
explicitly.
Three gaps in the parenthesization fix, all of the same class:
- A FieldRef whose FieldDef carries a child renders a JSON path chain
("data"->'age'), structurally an operator expression like JsonPathText, but
Expr::Field was not in the predicate. Cast(Field(data->age), "bigint") still
produced "data"->'age'::bigint — the original bug, reached through the other
spelling of a JSON path. Plain fields are bare identifiers and stay unbracketed.
- PostgreSQL's ?| / ?& fed their right operand straight into render_expr, with a
trailing ::text[] on top of it; SQLite's IRegex fed its right operand into the
RHS of a || concatenation. Both now go through render_operand.
- delegate_renderer! had no arm for render_operand, so a wrapping renderer fell
back to the trait default and lost the inner dialect's rule.
The predicate, not render_operand, is now the extension point:
Renderer::needs_operand_parens() defaults to Expr::needs_operand_parens(), and
SQLite overrides only that to exempt power()/XOR, which delimit themselves. The
bracketing logic exists once, so a change to the rule — like Field-with-child
above — reaches both dialects instead of silently bypassing SQLite.
Tests: reuse the existing render_expr_pg / render_expr_sqlite helpers instead of
rebuilding QueryStmt, drop the redundant sq_ prefixes, stop seeding tables for
SQLite queries with no FROM, and collapse four cloned PostgreSQL databases into
one for the precedence group.
Expr::Paren was a protheses for one decision: not bracketing Raw/Custom
operands. It earned its keep nowhere else, and it cost a lot.
A caller who needs grouping around a raw fragment simply writes it there —
Expr::raw("(price * qty)") — which is what they reach for anyway, and a
CustomExpr author controls their own rendering. So the variant bought nothing,
while it:
- defeated every shape check that matches on the right operand. IN with a
parenthesized array rendered `IN (?)` with the whole array bound as one
scalar (silently zero rows); BETWEEN rendered `BETWEEN (?)` with the AND half
missing, and bypassed the "requires exactly 2 values" guard. LIKE and IsNull
turned into render errors.
- made the release source-breaking (a new variant in a pub enum without
#[non_exhaustive] breaks every downstream exhaustive match), forcing 4.0.0.
- required an arm in every Expr walker, forever.
Also drop the render_operand arm from delegate_renderer!: it re-roots dispatch
at the inner renderer, so a wrapper's render_expr override is skipped in operand
positions. The trait default already recurses through self, which is correct
proxy behavior. The needs_operand_parens arm stays — that one is load-bearing.
And fix a test that asserted SQL PostgreSQL rejects: `?|` with a text concat
right operand renders `("a" || "b")::text[]`, which fails with `malformed array
literal`. The only meaningful compound operand there is an array concat, now
asserted and executed against a live PostgreSQL.
Expr::Custom and ConditionNode::Custom returned "must be handled by a wrapping
renderer" — but that wrapper could not be written.
delegate_renderer! emits every trait method, so combining it with an override is
a duplicate definition (E0201): the pattern in docs/extensibility.md never
compiled. And even a hand-written wrapper forwarding all 26 methods never saw
the call, because a dialect's render_query recurses through its own concrete
self and never comes back out. I confirmed both: the documented example fails to
compile, and a hand-rolled wrapper still returns Err(CustomExpr).
So the knowledge moves into the node. Every Custom* trait gains
fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx)
with an erroring default, and CustomExpr also gains needs_operand_parens()
(default true — conservative, since an infix node like `x AT TIME ZONE 'UTC'`
must be bracketed as the operand of a cast, which is exactly the bug class this
release exists to kill). The node is handed the renderer, so it recurses back
through it: sub-expressions, quoting and parameter numbering all flow through
the same dialect and the same RenderCtx.
No wrapper needed. PgVectorOp (<->, <#>, <=>, <+>) is now implemented through
the same public mechanism users get, replacing its hardcoded downcast.
Tested: a user-defined AT TIME ZONE node renders in a real query, is bracketed
under a cast, numbers its parameters in document order alongside the rest of the
statement, and executes against a live PostgreSQL. Custom binary operators and
custom conditions likewise. A node without render() still errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Renderers emitted no grouping, so an operand that was itself an operator expression got re-associated by the engine's precedence rules — two different ASTs rendered to the same SQL.
Cast(JsonPathText(data, 'age'), "bigint") produced
data->>'age'::bigint, which PostgreSQL reads asdata ->> ('age'::bigint)and rejects. Worse, Binary(Binary(1, +, 2), *, 3) produced1 + 2 * 3and silently evaluated to 7 instead of 9, with no error from the database.Operands of Binary, Unary, Cast, Collate, JsonPathText and of both sides of a comparison are now bracketed when they are themselves Binary, Unary, Collate, JsonPathText or Window. Bracketing is structural rather than driven by a precedence table, because precedence is dialect-specific: SQLite binds || tighter than *, PostgreSQL binds it looser than +.
Raw and Custom are never bracketed automatically — their contents are opaque and need not be an expression at all. The new Expr::Paren variant groups them explicitly.