From e9082d6652cbce64ab29a98bdca09c3760937ddd Mon Sep 17 00:00:00 2001 From: Emil Temirov Date: Tue, 14 Jul 2026 18:50:10 +0300 Subject: [PATCH 1/4] =?UTF-8?q?chore:=20release=203.2.0=20=E2=80=94=20pare?= =?UTF-8?q?nthesize=20operator=20operands,=20add=20Expr::Paren?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 19 ++ Cargo.lock | 8 +- Cargo.toml | 8 +- crates/qcraft-core/src/ast/expr.rs | 40 +++ crates/qcraft-core/src/render/renderer.rs | 18 + crates/qcraft-postgres/src/lib.rs | 27 +- crates/qcraft-postgres/tests/dql.rs | 312 +++++++++++++++++- .../qcraft-postgres/tests/integration/dql.rs | 153 +++++++++ crates/qcraft-sqlite/src/lib.rs | 47 ++- crates/qcraft-sqlite/tests/dql.rs | 172 +++++++++- crates/qcraft-sqlite/tests/integration_dql.rs | 67 ++++ docs/type-reference.md | 31 ++ 12 files changed, 873 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cd7384..68e5652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,24 @@ # Changelog +## 3.2.0 + +### Fixed +- **Operator operands are now parenthesized when their own structure requires it.** 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 with `invalid input syntax for type bigint`. 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 `+`, so the same AST needs different brackets per dialect. + +### Added +- `Expr::Paren(Box)` and the `Expr::paren(expr)` constructor — explicit grouping. Operator operands are bracketed automatically, so this is only needed to group an opaque `Raw` / `Custom` expression or to force brackets for readability. +- `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. + +### Changed +- Generated SQL now carries brackets where operand structure demands them (values are unchanged; only the SQL text differs). Snapshot tests that compare rendered SQL byte-for-byte may need updating — for example a `COLLATE` operand in a comparison now renders as `("users"."name" COLLATE "C") = $1`. +- Unary operators render via `keyword()` instead of `write()`, so `SELECT- x` is now `SELECT - x`. + +### Notes +- `Expr::Raw` and `Expr::Custom` are **never** bracketed automatically: their contents are opaque and need not be an expression at all. Wrap them in `Expr::Paren` when they need grouping — e.g. `Expr::cast(Expr::paren(Expr::raw("a + b")), "text")` renders `(a + b)::text`, while the unwrapped form renders `a + b::text`. +- Adding the `Expr::Paren` variant is source-breaking for exhaustive `match` over `Expr`. + ## 3.1.0 ### Added diff --git a/Cargo.lock b/Cargo.lock index f5fc4fd..ce0e9d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1831,7 +1831,7 @@ dependencies = [ [[package]] name = "qcraft" -version = "3.1.0" +version = "3.2.0" dependencies = [ "qcraft-core", "qcraft-postgres", @@ -1850,14 +1850,14 @@ dependencies = [ [[package]] name = "qcraft-core" -version = "3.1.0" +version = "3.2.0" dependencies = [ "thiserror", ] [[package]] name = "qcraft-postgres" -version = "3.1.0" +version = "3.2.0" dependencies = [ "libc", "postgres", @@ -1869,7 +1869,7 @@ dependencies = [ [[package]] name = "qcraft-sqlite" -version = "3.1.0" +version = "3.2.0" dependencies = [ "qcraft-core", "rusqlite", diff --git a/Cargo.toml b/Cargo.toml index 69aa374..e4b8635 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ members = [ ] [workspace.package] -version = "3.1.0" +version = "3.2.0" edition = "2024" rust-version = "1.85" license = "MIT OR Apache-2.0" @@ -19,7 +19,7 @@ categories = ["database"] readme = "README.md" [workspace.dependencies] -qcraft-core = { path = "crates/qcraft-core", version = "3.1.0" } -qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.1.0" } -qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.1.0" } +qcraft-core = { path = "crates/qcraft-core", version = "3.2.0" } +qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.2.0" } +qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.2.0" } thiserror = "2" diff --git a/crates/qcraft-core/src/ast/expr.rs b/crates/qcraft-core/src/ast/expr.rs index f01129c..1f12933 100644 --- a/crates/qcraft-core/src/ast/expr.rs +++ b/crates/qcraft-core/src/ast/expr.rs @@ -50,6 +50,13 @@ pub enum Expr { /// Collation override: `expr COLLATE "name"`. Collate { expr: Box, collation: String }, + /// Explicit grouping: `(expr)`. + /// + /// Operator operands are bracketed automatically (see [`Expr::needs_operand_parens`]), + /// so this is only needed to group an opaque `Raw`/`Custom` expression or to force + /// brackets for readability. + Paren(Box), + /// Build a JSON array: PG `jsonb_build_array(...)`, SQLite `json_array(...)`. JsonArray(Vec), @@ -273,6 +280,37 @@ impl Expr { Expr::Now } + /// Explicit grouping: `(expr)`. + pub fn paren(expr: Expr) -> Self { + Expr::Paren(Box::new(expr)) + } + + /// True if this expression must be parenthesized when it appears as the operand + /// of an operator (`+`, `::`, `COLLATE`, `->>`, a comparison, …). + /// + /// An operator expression carries its grouping in the tree, but SQL text carries + /// it in brackets: printed flat, `Binary(Binary(1, +, 2), *, 3)` becomes + /// `1 + 2 * 3`, which every engine reads back as `Binary(1, +, Binary(2, *, 3))` + /// — 7 instead of 9. Bracketing is structural rather than driven by a precedence + /// table because precedence is dialect-specific: SQLite binds `||` tighter than + /// `*`, PostgreSQL binds it looser than `+`. + /// + /// Self-delimiting forms (literals, identifiers, function calls, `CAST(…)`, + /// `CASE … END`, subqueries, tuples, [`Expr::Paren`]) carry their own boundaries + /// and render bare. `Raw` and `Custom` are opaque escape hatches whose contents + /// need not be an expression at all, so they are never bracketed automatically — + /// wrap them in [`Expr::Paren`] when they need grouping. + pub fn needs_operand_parens(&self) -> bool { + matches!( + self, + Expr::Binary { .. } + | Expr::Unary { .. } + | Expr::Collate { .. } + | Expr::JsonPathText { .. } + | Expr::Window(_) + ) + } + /// True if this expression tree contains an unbound `Expr::Param` placeholder. /// Used to reject double-render forms that would corrupt positional binding. /// Does not descend into subquery `QueryStmt`s (those are rejected separately). @@ -289,6 +327,7 @@ impl Expr { | Expr::Cast { expr, .. } | Expr::Collate { expr, .. } | Expr::JsonPathText { expr, .. } => expr.contains_unbound_param(), + Expr::Paren(expr) => expr.contains_unbound_param(), Expr::Func { args, .. } | Expr::Tuple(args) | Expr::JsonArray(args) => { args.iter().any(|a| a.contains_unbound_param()) } @@ -365,6 +404,7 @@ impl Expr { | Expr::Cast { expr, .. } | Expr::Collate { expr, .. } | Expr::JsonPathText { expr, .. } => expr.contains_subquery(), + Expr::Paren(expr) => expr.contains_subquery(), Expr::Func { args, .. } | Expr::Tuple(args) | Expr::JsonArray(args) => { args.iter().any(|a| a.contains_subquery()) } diff --git a/crates/qcraft-core/src/render/renderer.rs b/crates/qcraft-core/src/render/renderer.rs index e4e7ebd..5ad2b37 100644 --- a/crates/qcraft-core/src/render/renderer.rs +++ b/crates/qcraft-core/src/render/renderer.rs @@ -41,6 +41,24 @@ pub trait Renderer { // ── Expressions ── fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()>; + + /// Render `expr` where it is the operand of an operator (`+`, `::`, `COLLATE`, + /// `->>`, a comparison, …), bracketing it when its own structure would otherwise + /// be re-associated by the engine's operator precedence. + /// + /// See [`Expr::needs_operand_parens`] for which forms are bracketed and why this + /// is structural rather than precedence-driven. + fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { + if expr.needs_operand_parens() { + ctx.paren_open(); + self.render_expr(expr, ctx)?; + ctx.paren_close(); + Ok(()) + } else { + self.render_expr(expr, ctx) + } + } + fn render_aggregate(&self, agg: &AggregationDef, ctx: &mut RenderCtx) -> RenderResult<()>; fn render_window(&self, win: &WindowDef, ctx: &mut RenderCtx) -> RenderResult<()>; fn render_case(&self, case: &CaseDef, ctx: &mut RenderCtx) -> RenderResult<()>; diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index 109ca0f..d35bf7b 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -913,7 +913,7 @@ impl Renderer for PostgresRenderer { } Expr::Binary { left, op, right } => { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; // When using %s placeholders (psycopg), literal '%' must be // escaped as '%%' so the driver doesn't treat it as a placeholder. let mod_op = if self.param_style == ParamStyle::Percent { @@ -943,16 +943,16 @@ impl Renderer for PostgresRenderer { }); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } Expr::Unary { op, expr: inner } => { match op { - UnaryOp::Neg => ctx.write("-"), + UnaryOp::Neg => ctx.keyword("-"), UnaryOp::Not => ctx.keyword("NOT"), - UnaryOp::BitwiseNot => ctx.write("~"), + UnaryOp::BitwiseNot => ctx.keyword("~"), }; - self.render_expr(inner, ctx) + self.render_operand(inner, ctx) } Expr::Func { name, args } => { @@ -973,7 +973,7 @@ impl Renderer for PostgresRenderer { expr: inner, to_type, } => { - self.render_expr(inner, ctx)?; + self.render_operand(inner, ctx)?; ctx.operator("::"); ctx.write(to_type); Ok(()) @@ -1005,11 +1005,18 @@ impl Renderer for PostgresRenderer { } Expr::Collate { expr, collation } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.keyword("COLLATE").ident(collation); Ok(()) } + Expr::Paren(inner) => { + ctx.paren_open(); + self.render_expr(inner, ctx)?; + ctx.paren_close(); + Ok(()) + } + Expr::JsonArray(items) => { ctx.keyword("jsonb_build_array").write("("); for (i, item) in items.iter().enumerate() { @@ -1104,7 +1111,7 @@ impl Renderer for PostgresRenderer { } Expr::JsonPathText { expr, path } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.operator("->>'") .write(&path.replace('\'', "''")) .write("'"); @@ -1284,7 +1291,7 @@ impl Renderer for PostgresRenderer { right: &Expr, ctx: &mut RenderCtx, ) -> RenderResult<()> { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; match op { CompareOp::Eq => ctx.write(" = "), CompareOp::Neq => ctx.write(" <> "), @@ -1406,7 +1413,7 @@ impl Renderer for PostgresRenderer { )); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } // ── Query (stub) ───────────────────────────────────────────────────── diff --git a/crates/qcraft-postgres/tests/dql.rs b/crates/qcraft-postgres/tests/dql.rs index 0fd4ab2..b5bdef1 100644 --- a/crates/qcraft-postgres/tests/dql.rs +++ b/crates/qcraft-postgres/tests/dql.rs @@ -1802,7 +1802,7 @@ fn collate_in_where() { }); assert_eq!( sql, - r#"SELECT * FROM "users" WHERE "users"."name" COLLATE "und-x-icu" = $1"# + r#"SELECT * FROM "users" WHERE ("users"."name" COLLATE "und-x-icu") = $1"# ); assert_eq!(params, vec![Value::Str("alice".into())]); } @@ -2758,3 +2758,313 @@ fn is_null_non_boolean_right_errors() { .to_string(); assert!(err.contains("IsNull"), "unexpected error: {err}"); } + +// ========================================================================== +// CAST operand parenthesization — `::` binds tighter than every operator, +// so a compound operand must be wrapped or the cast lands on a sub-part. +// ========================================================================== + +fn cast_sql(inner: Expr, to_type: &str) -> String { + render(&QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(inner, to_type), + alias: None, + }], + from: Some(users_from()), + ..simple_query() + }) +} + +#[test] +fn cast_over_json_path_text_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::JsonPathText { + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + path: "age".into(), + }, + "bigint", + ), + r#"SELECT ("users"."data"->>'age')::bigint FROM "users""# + ); +} + +#[test] +fn cast_over_binary_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Binary { + left: Box::new(Expr::Field(FieldRef::new("users", "id"))), + op: qcraft_core::ast::expr::BinaryOp::Add, + right: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + r#"SELECT ("users"."id" + "users"."age")::text FROM "users""# + ); +} + +#[test] +fn cast_over_unary_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Unary { + op: qcraft_core::ast::expr::UnaryOp::Neg, + expr: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + r#"SELECT (- "users"."age")::text FROM "users""# + ); +} + +#[test] +fn cast_over_collate_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "name"))), + collation: "C".into(), + }, + "text", + ), + r#"SELECT ("users"."name" COLLATE "C")::text FROM "users""# + ); +} + +#[test] +fn cast_over_raw_stays_bare() { + // Raw is an opaque escape hatch: its contents may not even be an expression, + // so the renderer never brackets it. Callers who need grouping wrap it in + // Expr::Paren themselves. + assert_eq!( + cast_sql(Expr::raw("a + b"), "text"), + r#"SELECT a + b::text FROM "users""# + ); +} + +#[test] +fn cast_over_parenthesized_raw_is_grouped() { + assert_eq!( + cast_sql(Expr::paren(Expr::raw("a + b")), "text"), + r#"SELECT (a + b)::text FROM "users""# + ); +} + +#[test] +fn paren_wraps_any_expression() { + assert_eq!( + expr_sql(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), + r#"SELECT ("users"."age") FROM "users""# + ); +} + +#[test] +fn paren_operand_is_not_double_wrapped() { + use qcraft_core::ast::expr::BinaryOp; + // Paren is self-delimiting: it must not attract a second pair of brackets. + assert_eq!( + expr_sql(bin( + Expr::paren(bin(int(1), BinaryOp::Add, int(2))), + BinaryOp::Mul, + int(3), + )), + r#"SELECT (1 + 2) * 3 FROM "users""# + ); +} + +#[test] +fn cast_over_field_stays_bare() { + assert_eq!( + cast_sql(Expr::Field(FieldRef::new("users", "age")), "text"), + r#"SELECT "users"."age"::text FROM "users""# + ); +} + +#[test] +fn cast_over_func_stays_bare() { + assert_eq!( + cast_sql( + Expr::func("lower", vec![Expr::Field(FieldRef::new("users", "name"))]), + "text", + ), + r#"SELECT lower("users"."name")::text FROM "users""# + ); +} + +#[test] +fn cast_over_cast_stays_bare() { + assert_eq!( + cast_sql( + Expr::cast(Expr::Field(FieldRef::new("users", "age")), "text"), + "integer", + ), + r#"SELECT "users"."age"::text::integer FROM "users""# + ); +} + +#[test] +fn cast_over_tuple_stays_bare() { + assert_eq!( + cast_sql( + Expr::Tuple(vec![ + Expr::Field(FieldRef::new("users", "id")), + Expr::Field(FieldRef::new("users", "name")), + ]), + "text", + ), + r#"SELECT ("users"."id", "users"."name")::text FROM "users""# + ); +} + +// ========================================================================== +// Operator precedence — a compound operand must be parenthesized, otherwise +// PostgreSQL re-associates the expression by its own precedence rules. +// ========================================================================== + +fn expr_sql(expr: Expr) -> String { + render(&QueryStmt { + columns: vec![SelectColumn::Expr { expr, alias: None }], + from: Some(users_from()), + ..simple_query() + }) +} + +fn bin(left: Expr, op: qcraft_core::ast::expr::BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn int(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +#[test] +fn nested_binary_left_operand_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // (1 + 2) * 3 — bare `1 + 2 * 3` would be 7, not 9. + assert_eq!( + expr_sql(bin( + bin(int(1), BinaryOp::Add, int(2)), + BinaryOp::Mul, + int(3) + )), + r#"SELECT (1 + 2) * 3 FROM "users""# + ); +} + +#[test] +fn nested_binary_right_operand_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // 10 - (5 - 2) — bare `10 - 5 - 2` is left-associative, giving 3, not 7. + assert_eq!( + expr_sql(bin( + int(10), + BinaryOp::Sub, + bin(int(5), BinaryOp::Sub, int(2)) + )), + r#"SELECT 10 - (5 - 2) FROM "users""# + ); +} + +#[test] +fn unary_over_binary_is_parenthesized() { + use qcraft_core::ast::expr::{BinaryOp, UnaryOp}; + // -(2 + 3) — bare `- 2 + 3` binds the minus to 2, giving 1, not -5. + assert_eq!( + expr_sql(Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(int(2), BinaryOp::Add, int(3))), + }), + r#"SELECT - (2 + 3) FROM "users""# + ); +} + +#[test] +fn collate_over_binary_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // COLLATE binds tighter than ||, so a bare operand collates only the right side. + assert_eq!( + expr_sql(Expr::Collate { + expr: Box::new(bin( + Expr::Field(FieldRef::new("users", "name")), + BinaryOp::Concat, + Expr::Field(FieldRef::new("users", "department")), + )), + collation: "C".into(), + }), + r#"SELECT ("users"."name" || "users"."department") COLLATE "C" FROM "users""# + ); +} + +#[test] +fn json_path_text_over_unary_not_is_parenthesized() { + use qcraft_core::ast::expr::UnaryOp; + // NOT is weaker than ->>, so a bare operand negates the extraction instead. + assert_eq!( + expr_sql(Expr::JsonPathText { + expr: Box::new(Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + }), + path: "k".into(), + }), + r#"SELECT (NOT "users"."data")->>'k' FROM "users""# + ); +} + +#[test] +fn cast_over_nested_binary_parenthesizes_both_levels() { + use qcraft_core::ast::expr::BinaryOp; + // ((1 + 2) * 3)::bigint — the cast wraps the whole operand AND the inner + // sum keeps its own parens, otherwise the value is 7 instead of 9. + assert_eq!( + expr_sql(Expr::cast( + bin(bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, int(3)), + "bigint", + )), + r#"SELECT ((1 + 2) * 3)::bigint FROM "users""# + ); +} + +#[test] +fn atomic_binary_operands_stay_bare() { + use qcraft_core::ast::expr::BinaryOp; + // Fields, literals, calls and casts are self-delimiting — no parens added. + assert_eq!( + expr_sql(bin( + Expr::Field(FieldRef::new("users", "id")), + BinaryOp::Add, + Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), + )), + r#"SELECT "users"."id" + abs("users"."age") FROM "users""# + ); +} + +#[test] +fn comparison_with_not_operand_is_parenthesized() { + use qcraft_core::ast::expr::UnaryOp; + // NOT is weaker than `=`, so `NOT x = y` parses as NOT (x = y). + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "active"))), + }, + CompareOp::Eq, + Expr::Value(Value::Bool(true)), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE (NOT "users"."active") = $1"# + ); +} diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index 4c91a1b..484fc90 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -2351,3 +2351,156 @@ fn collate_in_where_comparison() { // 'A'(65) 'B'(66) 'C'(67) < 'D'(68) — Alice, Bob, Charlie match assert_eq!(names, vec!["Alice", "Bob", "Charlie"]); } + +// ========================================================================== +// CAST over a compound operand — `::` binds tighter than `->>` and than the +// arithmetic operators, so an unparenthesized operand casts the wrong node. +// ========================================================================== + +#[test] +fn cast_over_json_path_text_executes() { + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "people" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "people" VALUES (1, '{"age": 36}'::jsonb)"#, + &[], + ) + .unwrap(); + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast( + Expr::JsonPathText { + expr: Box::new(Expr::Field(FieldRef::new("people", "data"))), + path: "age".into(), + }, + "bigint", + ), + alias: Some("age".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("people"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, i64>(0), 36); +} + +#[test] +fn cast_over_binary_executes() { + let mut client = crate::test_client("template_dql"); + // "id" + "age" for Alice is 1 + 30; a bare `::text` would cast only "age", + // leaving `1 + '30'` — an int + text mismatch that PostgreSQL rejects. + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast( + Expr::Binary { + left: Box::new(Expr::Field(FieldRef::new("users", "id"))), + op: BinaryOp::Add, + right: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + alias: Some("sum_text".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("users"))]), + where_clause: Some(simple_cond_eq( + Expr::Field(FieldRef::new("users", "id")), + Expr::Value(Value::Int(1)), + )), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, String>(0), "31"); +} + +// ========================================================================== +// Operator precedence — executed against PostgreSQL, asserting the VALUE. +// Without parens these queries succeed but compute the wrong number. +// ========================================================================== + +fn lit(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn scalar_i64(expr: Expr) -> i64 { + let mut client = crate::test_client("template_dql"); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { expr, alias: None }], + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + let rows = client.query(&sql, ¶ms).unwrap(); + rows[0].get::<_, i64>(0) +} + +#[test] +fn precedence_nested_binary_executes() { + // (1 + 2) * 3 = 9; flat `1 + 2 * 3` silently yields 7. + let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); + assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), 9); +} + +#[test] +fn precedence_right_operand_associativity_executes() { + // 10 - (5 - 2) = 7; flat `10 - 5 - 2` is left-associative and yields 3. + let expr = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); + assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), 7); +} + +#[test] +fn precedence_unary_over_binary_executes() { + // -(2 + 3) = -5; flat `- 2 + 3` binds the minus to 2 and yields 1. + let expr = Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(lit(2), BinaryOp::Add, lit(3))), + }; + assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), -5); +} + +#[test] +fn precedence_collate_over_binary_executes() { + // COLLATE binds tighter than ||, so a flat operand collates the integer 5 + // and PostgreSQL errors with "collations are not supported by type integer". + let mut client = crate::test_client("template_dql"); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Collate { + expr: Box::new(bin(Expr::raw("'x'"), BinaryOp::Concat, Expr::raw("5"))), + collation: "C".into(), + }, + alias: None, + }], + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows[0].get::<_, String>(0), "x5"); +} diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index 67e4771..69b410e 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -571,7 +571,7 @@ impl Renderer for SqliteRenderer { // Everything else stays infix. _ => { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; ctx.keyword(match op { BinaryOp::Add => "+", BinaryOp::Sub => "-", @@ -586,17 +586,17 @@ impl Renderer for SqliteRenderer { BinaryOp::Power | BinaryOp::BitwiseXor => unreachable!(), BinaryOp::Custom(_) => unreachable!(), }); - self.render_expr(right, ctx) + self.render_operand(right, ctx) } }, Expr::Unary { op, expr: inner } => { match op { - UnaryOp::Neg => ctx.write("-"), + UnaryOp::Neg => ctx.keyword("-"), UnaryOp::Not => ctx.keyword("NOT"), - UnaryOp::BitwiseNot => ctx.write("~"), + UnaryOp::BitwiseNot => ctx.keyword("~"), }; - self.render_expr(inner, ctx) + self.render_operand(inner, ctx) } Expr::Func { name, args } => { @@ -646,11 +646,18 @@ impl Renderer for SqliteRenderer { )), Expr::Collate { expr, collation } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.keyword("COLLATE").keyword(collation); Ok(()) } + Expr::Paren(inner) => { + ctx.paren_open(); + self.render_expr(inner, ctx)?; + ctx.paren_close(); + Ok(()) + } + Expr::JsonArray(items) => { ctx.keyword("json_array").write("("); for (i, item) in items.iter().enumerate() { @@ -748,7 +755,7 @@ impl Renderer for SqliteRenderer { } Expr::JsonPathText { expr, path } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.operator("->>'") .write(&path.replace('\'', "''")) .write("'"); @@ -914,6 +921,28 @@ impl Renderer for SqliteRenderer { Ok(()) } + /// SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed + /// composite, so those two are already self-delimiting and must not collect a + /// second pair of brackets. Every other operand follows the shared rule. + fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { + if matches!( + expr, + Expr::Binary { + op: BinaryOp::Power | BinaryOp::BitwiseXor, + .. + } + ) { + return self.render_expr(expr, ctx); + } + if expr.needs_operand_parens() { + ctx.paren_open(); + self.render_expr(expr, ctx)?; + ctx.paren_close(); + return Ok(()); + } + self.render_expr(expr, ctx) + } + fn render_compare_op( &self, op: &CompareOp, @@ -928,7 +957,7 @@ impl Renderer for SqliteRenderer { if needs_lower { ctx.keyword("LOWER").write("("); } - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; if needs_lower { ctx.paren_close(); } @@ -1049,7 +1078,7 @@ impl Renderer for SqliteRenderer { )); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } // ── Query (stub) ───────────────────────────────────────────────────── diff --git a/crates/qcraft-sqlite/tests/dql.rs b/crates/qcraft-sqlite/tests/dql.rs index d1f2f14..6472a5e 100644 --- a/crates/qcraft-sqlite/tests/dql.rs +++ b/crates/qcraft-sqlite/tests/dql.rs @@ -1179,7 +1179,7 @@ fn collate_in_where() { }); assert_eq!( sql, - r#"SELECT * FROM "users" WHERE "users"."name" COLLATE NOCASE = ?"# + r#"SELECT * FROM "users" WHERE ("users"."name" COLLATE NOCASE) = ?"# ); assert_eq!(params, vec![Value::Str("alice".into())]); } @@ -2067,3 +2067,173 @@ fn is_null_non_boolean_right_errors() { let err = render_err(&stmt); assert!(err.contains("IsNull"), "unexpected error: {err}"); } + +// ========================================================================== +// Operator precedence — a compound operand must be parenthesized, otherwise +// SQLite re-associates the expression by its own precedence rules (which, +// for `||` vs `* /`, differ from PostgreSQL's). +// ========================================================================== + +fn sq_expr_sql(expr: Expr) -> String { + render(&QueryStmt { + columns: vec![SelectColumn::Expr { expr, alias: None }], + ..simple_query() + }) +} + +fn sq_bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn sq_int(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +#[test] +fn nested_binary_left_operand_is_parenthesized() { + // (1 + 2) * 3 — bare `1 + 2 * 3` would be 7, not 9. + assert_eq!( + sq_expr_sql(sq_bin( + sq_bin(sq_int(1), BinaryOp::Add, sq_int(2)), + BinaryOp::Mul, + sq_int(3), + )), + r#"SELECT (1 + 2) * 3 FROM "users""# + ); +} + +#[test] +fn nested_binary_right_operand_is_parenthesized() { + // 10 - (5 - 2) — bare `10 - 5 - 2` is left-associative, giving 3, not 7. + assert_eq!( + sq_expr_sql(sq_bin( + sq_int(10), + BinaryOp::Sub, + sq_bin(sq_int(5), BinaryOp::Sub, sq_int(2)), + )), + r#"SELECT 10 - (5 - 2) FROM "users""# + ); +} + +#[test] +fn unary_over_binary_is_parenthesized() { + // -(2 + 3) — bare `- 2 + 3` binds the minus to 2, giving 1, not -5. + assert_eq!( + sq_expr_sql(Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(sq_bin(sq_int(2), BinaryOp::Add, sq_int(3))), + }), + r#"SELECT - (2 + 3) FROM "users""# + ); +} + +#[test] +fn collate_over_binary_is_parenthesized() { + assert_eq!( + sq_expr_sql(Expr::Collate { + expr: Box::new(sq_bin( + Expr::Field(FieldRef::new("users", "name")), + BinaryOp::Concat, + Expr::Field(FieldRef::new("users", "department")), + )), + collation: "NOCASE".into(), + }), + r#"SELECT ("users"."name" || "users"."department") COLLATE NOCASE FROM "users""# + ); +} + +#[test] +fn json_path_text_over_unary_not_is_parenthesized() { + assert_eq!( + sq_expr_sql(Expr::JsonPathText { + expr: Box::new(Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + }), + path: "k".into(), + }), + r#"SELECT (NOT "users"."data")->>'k' FROM "users""# + ); +} + +#[test] +fn cast_over_nested_binary_keeps_inner_parens() { + // CAST(...) is self-delimiting, but the inner sum still needs its own parens. + assert_eq!( + sq_expr_sql(Expr::cast( + sq_bin( + sq_bin(sq_int(1), BinaryOp::Add, sq_int(2)), + BinaryOp::Mul, + sq_int(3), + ), + "INTEGER", + )), + r#"SELECT CAST((1 + 2) * 3 AS INTEGER) FROM "users""# + ); +} + +#[test] +fn atomic_binary_operands_stay_bare() { + assert_eq!( + sq_expr_sql(sq_bin( + Expr::Field(FieldRef::new("users", "id")), + BinaryOp::Add, + Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), + )), + r#"SELECT "users"."id" + abs("users"."age") FROM "users""# + ); +} + +#[test] +fn comparison_with_not_operand_is_parenthesized() { + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "active"))), + }, + CompareOp::Eq, + Expr::Value(Value::Bool(true)), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE (NOT "users"."active") = ?"# + ); +} + +#[test] +fn cast_over_raw_stays_bare() { + // Raw is an opaque escape hatch — never bracketed automatically. + assert_eq!( + sq_expr_sql(Expr::cast(Expr::raw("a + b"), "INTEGER")), + r#"SELECT CAST(a + b AS INTEGER) FROM "users""# + ); +} + +#[test] +fn paren_operand_is_not_double_wrapped() { + assert_eq!( + sq_expr_sql(sq_bin( + Expr::paren(sq_bin(sq_int(1), BinaryOp::Add, sq_int(2))), + BinaryOp::Mul, + sq_int(3), + )), + r#"SELECT (1 + 2) * 3 FROM "users""# + ); +} + +#[test] +fn paren_wraps_any_expression() { + assert_eq!( + sq_expr_sql(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), + r#"SELECT ("users"."age") FROM "users""# + ); +} diff --git a/crates/qcraft-sqlite/tests/integration_dql.rs b/crates/qcraft-sqlite/tests/integration_dql.rs index 0a8a704..38bc4ff 100644 --- a/crates/qcraft-sqlite/tests/integration_dql.rs +++ b/crates/qcraft-sqlite/tests/integration_dql.rs @@ -2239,3 +2239,70 @@ fn sqlite_xor_computes_real_xor_via_rusqlite() { assert_eq!(got, a ^ b, "sql={sql}"); } } + +// ========================================================================== +// Operator precedence — executed against SQLite, asserting the VALUE. +// SQLite's precedence differs from PostgreSQL's (`||` binds tighter than +// `* /` here), which is exactly why operands must carry their own parens. +// ========================================================================== + +fn lit(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn scalar(expr: Expr) -> T { + let conn = Connection::open_in_memory().unwrap(); + setup_db(&conn); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { expr, alias: None }], + from: None, + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = common::to_sqlite_params(&values); + let params = common::as_sqlite_params(&boxed); + conn.query_row(&sql, params.as_slice(), |row| row.get(0)) + .unwrap() +} + +#[test] +fn precedence_nested_binary_executes() { + // (1 + 2) * 3 = 9; flat `1 + 2 * 3` silently yields 7. + let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); + assert_eq!(scalar::(expr), 9); +} + +#[test] +fn precedence_right_operand_associativity_executes() { + // 10 - (5 - 2) = 7; flat `10 - 5 - 2` is left-associative and yields 3. + let expr = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); + assert_eq!(scalar::(expr), 7); +} + +#[test] +fn precedence_unary_over_binary_executes() { + // -(2 + 3) = -5; flat `- 2 + 3` binds the minus to 2 and yields 1. + let expr = Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(lit(2), BinaryOp::Add, lit(3))), + }; + assert_eq!(scalar::(expr), -5); +} + +#[test] +fn precedence_concat_over_sum_executes() { + // (1 + 2) || 3 = '33'. In SQLite `||` binds TIGHTER than `+`, so the flat + // form `1 + 2 || 3` parses as 1 + ('2' || '3') = 24 — the same AST that + // PostgreSQL would render correctly without parens. Dialect-specific + // precedence is why operands must be parenthesized structurally. + let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Concat, lit(3)); + assert_eq!(scalar::(expr), "33"); +} diff --git a/docs/type-reference.md b/docs/type-reference.md index 51af317..6186a87 100644 --- a/docs/type-reference.md +++ b/docs/type-reference.md @@ -83,6 +83,7 @@ pub enum Expr { SubQuery(Box), ArraySubQuery(Box), Collate { expr: Box, collation: String }, + Paren(Box), Raw { sql: String, params: Vec }, JsonArray(Vec), JsonObject(Vec<(String, Expr)>), @@ -111,6 +112,7 @@ pub enum Expr { | `Expr::exists(query)` | `Expr::Exists(Box::new(query))` | | `Expr::subquery(query)` | `Expr::SubQuery(Box::new(query))` | | `expr.collate("C")` | `Expr::Collate { expr, collation: "C" }` | +| `Expr::paren(expr)` | `Expr::Paren(Box::new(expr))` — explicit grouping | | `Expr::json_array(vec![...])` | `Expr::JsonArray(...)` — PG: `jsonb_build_array`, SQLite: `json_array` | | `Expr::json_object(vec![...])` | `Expr::JsonObject(...)` — PG: `jsonb_build_object`, SQLite: `json_object` | | `Expr::json_agg(expr)` | `Expr::JsonAgg { ... }` — PG: `jsonb_agg`, SQLite: `json_group_array` | @@ -127,6 +129,35 @@ pub enum Expr { | `Value` | `Expr::Value(v)` | | `FieldRef` | `Expr::Field(f)` | +### Parenthesization + +The tree carries the grouping, so the renderer brackets an operand whenever its own +structure would otherwise be re-associated by the engine's operator precedence: + +```rust +// (1 + 2) * 3 — flat `1 + 2 * 3` would evaluate to 7 +Expr::Binary { + left: Box::new(Expr::Binary { left: one, op: BinaryOp::Add, right: two }), + op: BinaryOp::Mul, + right: three, +} +// PG and SQLite: (1 + 2) * 3 +``` + +Operands that are `Binary`, `Unary`, `Collate`, `JsonPathText` or `Window` get brackets; +self-delimiting forms (literals, fields, function calls, `CAST(…)`, `CASE … END`, +subqueries, tuples, `Paren`) render bare. Bracketing is structural rather than driven by a +precedence table, because precedence differs per dialect — 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. Wrap them yourself when they need grouping: + +```rust +Expr::cast(Expr::raw("a + b"), "text") // a + b::text ← cast binds to b +Expr::cast(Expr::paren(Expr::raw("a + b")), "text") // (a + b)::text +``` + ## FieldRef / FieldDef A field reference with optional schema namespace and JSON child path. From 94195a5a7ef5a42d11d86eddc5adebd3b75c7c69 Mon Sep 17 00:00:00 2001 From: Emil Temirov Date: Tue, 14 Jul 2026 19:27:15 +0300 Subject: [PATCH 2/4] fix: cover the operand positions the first pass missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 8 + crates/qcraft-core/src/ast/expr.rs | 20 +- crates/qcraft-core/src/render/renderer.rs | 20 +- crates/qcraft-postgres/src/lib.rs | 4 +- crates/qcraft-postgres/tests/dql.rs | 113 +++++++---- .../qcraft-postgres/tests/integration/dql.rs | 107 +++++++---- crates/qcraft-sqlite/src/lib.rs | 16 +- crates/qcraft-sqlite/tests/dql.rs | 176 +++++++++++++----- crates/qcraft-sqlite/tests/integration_dql.rs | 2 +- 9 files changed, 337 insertions(+), 129 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68e5652..678bb56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,17 @@ Bracketing is structural rather than driven by a precedence table, because precedence is dialect-specific: SQLite binds `||` tighter than `*`, PostgreSQL binds it looser than `+`, so the same AST needs different brackets per dialect. + The same defect reached through `Expr::Field`: a `FieldDef` carrying a `child` renders a JSON path chain (`"data"->'age'`), so `Cast(Field(data->age), "bigint")` produced `"data"->'age'::bigint` — bracketed now as well. Plain fields are bare identifiers and stay unbracketed. + + Two further operand positions were missed on the first pass and are now covered: the right operand of PostgreSQL's `?|` / `?&` (which also feeds a trailing `::text[]`), and the right operand of SQLite's `IRegex` (the RHS of a `||` concatenation). + ### Added - `Expr::Paren(Box)` and the `Expr::paren(expr)` constructor — explicit grouping. Operator operands are bracketed automatically, so this is only needed to group an opaque `Raw` / `Custom` expression or to force brackets for readability. - `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. +- `Renderer::needs_operand_parens()` — the extension point a dialect overrides when its own rendering of a node already delimits it (SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed composite, so neither takes a second pair of brackets). Overriding the predicate rather than `render_operand` keeps the bracketing logic single-sourced. + +### Fixed (internal) +- `delegate_renderer!` now forwards `render_operand` and `needs_operand_parens`. Without those arms a wrapping renderer silently fell back to the trait default and lost the inner dialect's rule. ### Changed - Generated SQL now carries brackets where operand structure demands them (values are unchanged; only the SQL text differs). Snapshot tests that compare rendered SQL byte-for-byte may need updating — for example a `COLLATE` operand in a comparison now renders as `("users"."name" COLLATE "C") = $1`. diff --git a/crates/qcraft-core/src/ast/expr.rs b/crates/qcraft-core/src/ast/expr.rs index 1f12933..07cfaeb 100644 --- a/crates/qcraft-core/src/ast/expr.rs +++ b/crates/qcraft-core/src/ast/expr.rs @@ -295,20 +295,26 @@ impl Expr { /// table because precedence is dialect-specific: SQLite binds `||` tighter than /// `*`, PostgreSQL binds it looser than `+`. /// + /// A [`Expr::Field`] whose [`FieldDef`](crate::ast::common::FieldDef) carries a + /// `child` renders as a JSON path chain (`"data"->'age'`) — an operator expression + /// like [`Expr::JsonPathText`], and bracketed for the same reason. A plain field + /// is a bare identifier and is not. + /// /// Self-delimiting forms (literals, identifiers, function calls, `CAST(…)`, /// `CASE … END`, subqueries, tuples, [`Expr::Paren`]) carry their own boundaries /// and render bare. `Raw` and `Custom` are opaque escape hatches whose contents /// need not be an expression at all, so they are never bracketed automatically — /// wrap them in [`Expr::Paren`] when they need grouping. pub fn needs_operand_parens(&self) -> bool { - matches!( - self, + match self { Expr::Binary { .. } - | Expr::Unary { .. } - | Expr::Collate { .. } - | Expr::JsonPathText { .. } - | Expr::Window(_) - ) + | Expr::Unary { .. } + | Expr::Collate { .. } + | Expr::JsonPathText { .. } + | Expr::Window(_) => true, + Expr::Field(field_ref) => field_ref.field.child.is_some(), + _ => false, + } } /// True if this expression tree contains an unbound `Expr::Param` placeholder. diff --git a/crates/qcraft-core/src/render/renderer.rs b/crates/qcraft-core/src/render/renderer.rs index 5ad2b37..ac550e5 100644 --- a/crates/qcraft-core/src/render/renderer.rs +++ b/crates/qcraft-core/src/render/renderer.rs @@ -42,6 +42,14 @@ pub trait Renderer { fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()>; + /// Whether `expr` needs brackets in an operand position. Defaults to + /// [`Expr::needs_operand_parens`]; a dialect overrides this — not + /// [`Renderer::render_operand`] — when its own rendering of a node already + /// delimits it (SQLite renders `Power` as `power(l, r)`, for instance). + fn needs_operand_parens(&self, expr: &Expr) -> bool { + expr.needs_operand_parens() + } + /// Render `expr` where it is the operand of an operator (`+`, `::`, `COLLATE`, /// `->>`, a comparison, …), bracketing it when its own structure would otherwise /// be re-associated by the engine's operator precedence. @@ -49,7 +57,7 @@ pub trait Renderer { /// See [`Expr::needs_operand_parens`] for which forms are bracketed and why this /// is structural rather than precedence-driven. fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { - if expr.needs_operand_parens() { + if self.needs_operand_parens(expr) { ctx.paren_open(); self.render_expr(expr, ctx)?; ctx.paren_close(); @@ -194,6 +202,16 @@ macro_rules! delegate_renderer { ) -> $crate::error::RenderResult<()> { $self.$inner.render_expr(expr, ctx) } + fn needs_operand_parens(&$self, expr: &$crate::ast::expr::Expr) -> bool { + $self.$inner.needs_operand_parens(expr) + } + fn render_operand( + &$self, + expr: &$crate::ast::expr::Expr, + ctx: &mut $crate::render::ctx::RenderCtx, + ) -> $crate::error::RenderResult<()> { + $self.$inner.render_operand(expr, ctx) + } fn render_aggregate( &$self, agg: &$crate::ast::expr::AggregationDef, diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index d35bf7b..51dcf19 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -1366,13 +1366,13 @@ impl Renderer for PostgresRenderer { CompareOp::JsonbHasKey => ctx.write(" ? "), CompareOp::JsonbHasAnyKey => { ctx.write(" ?| "); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; ctx.write("::text[]"); return Ok(()); } CompareOp::JsonbHasAllKeys => { ctx.write(" ?& "); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; ctx.write("::text[]"); return Ok(()); } diff --git a/crates/qcraft-postgres/tests/dql.rs b/crates/qcraft-postgres/tests/dql.rs index b5bdef1..c5db5b4 100644 --- a/crates/qcraft-postgres/tests/dql.rs +++ b/crates/qcraft-postgres/tests/dql.rs @@ -2765,14 +2765,7 @@ fn is_null_non_boolean_right_errors() { // ========================================================================== fn cast_sql(inner: Expr, to_type: &str) -> String { - render(&QueryStmt { - columns: vec![SelectColumn::Expr { - expr: Expr::cast(inner, to_type), - alias: None, - }], - from: Some(users_from()), - ..simple_query() - }) + expr_sql(Expr::cast(inner, to_type)) } #[test] @@ -2785,7 +2778,7 @@ fn cast_over_json_path_text_is_parenthesized() { }, "bigint", ), - r#"SELECT ("users"."data"->>'age')::bigint FROM "users""# + r#"SELECT ("users"."data"->>'age')::bigint"# ); } @@ -2800,7 +2793,7 @@ fn cast_over_binary_is_parenthesized() { }, "text", ), - r#"SELECT ("users"."id" + "users"."age")::text FROM "users""# + r#"SELECT ("users"."id" + "users"."age")::text"# ); } @@ -2814,7 +2807,7 @@ fn cast_over_unary_is_parenthesized() { }, "text", ), - r#"SELECT (- "users"."age")::text FROM "users""# + r#"SELECT (- "users"."age")::text"# ); } @@ -2828,7 +2821,7 @@ fn cast_over_collate_is_parenthesized() { }, "text", ), - r#"SELECT ("users"."name" COLLATE "C")::text FROM "users""# + r#"SELECT ("users"."name" COLLATE "C")::text"# ); } @@ -2839,7 +2832,7 @@ fn cast_over_raw_stays_bare() { // Expr::Paren themselves. assert_eq!( cast_sql(Expr::raw("a + b"), "text"), - r#"SELECT a + b::text FROM "users""# + r#"SELECT a + b::text"# ); } @@ -2847,7 +2840,7 @@ fn cast_over_raw_stays_bare() { fn cast_over_parenthesized_raw_is_grouped() { assert_eq!( cast_sql(Expr::paren(Expr::raw("a + b")), "text"), - r#"SELECT (a + b)::text FROM "users""# + r#"SELECT (a + b)::text"# ); } @@ -2855,7 +2848,7 @@ fn cast_over_parenthesized_raw_is_grouped() { fn paren_wraps_any_expression() { assert_eq!( expr_sql(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), - r#"SELECT ("users"."age") FROM "users""# + r#"SELECT ("users"."age")"# ); } @@ -2869,7 +2862,7 @@ fn paren_operand_is_not_double_wrapped() { BinaryOp::Mul, int(3), )), - r#"SELECT (1 + 2) * 3 FROM "users""# + r#"SELECT (1 + 2) * 3"# ); } @@ -2877,7 +2870,7 @@ fn paren_operand_is_not_double_wrapped() { fn cast_over_field_stays_bare() { assert_eq!( cast_sql(Expr::Field(FieldRef::new("users", "age")), "text"), - r#"SELECT "users"."age"::text FROM "users""# + r#"SELECT "users"."age"::text"# ); } @@ -2888,7 +2881,7 @@ fn cast_over_func_stays_bare() { Expr::func("lower", vec![Expr::Field(FieldRef::new("users", "name"))]), "text", ), - r#"SELECT lower("users"."name")::text FROM "users""# + r#"SELECT lower("users"."name")::text"# ); } @@ -2899,7 +2892,7 @@ fn cast_over_cast_stays_bare() { Expr::cast(Expr::Field(FieldRef::new("users", "age")), "text"), "integer", ), - r#"SELECT "users"."age"::text::integer FROM "users""# + r#"SELECT "users"."age"::text::integer"# ); } @@ -2913,7 +2906,7 @@ fn cast_over_tuple_stays_bare() { ]), "text", ), - r#"SELECT ("users"."id", "users"."name")::text FROM "users""# + r#"SELECT ("users"."id", "users"."name")::text"# ); } @@ -2923,11 +2916,7 @@ fn cast_over_tuple_stays_bare() { // ========================================================================== fn expr_sql(expr: Expr) -> String { - render(&QueryStmt { - columns: vec![SelectColumn::Expr { expr, alias: None }], - from: Some(users_from()), - ..simple_query() - }) + render_expr_pg(expr).0 } fn bin(left: Expr, op: qcraft_core::ast::expr::BinaryOp, right: Expr) -> Expr { @@ -2952,7 +2941,7 @@ fn nested_binary_left_operand_is_parenthesized() { BinaryOp::Mul, int(3) )), - r#"SELECT (1 + 2) * 3 FROM "users""# + r#"SELECT (1 + 2) * 3"# ); } @@ -2966,7 +2955,7 @@ fn nested_binary_right_operand_is_parenthesized() { BinaryOp::Sub, bin(int(5), BinaryOp::Sub, int(2)) )), - r#"SELECT 10 - (5 - 2) FROM "users""# + r#"SELECT 10 - (5 - 2)"# ); } @@ -2979,7 +2968,7 @@ fn unary_over_binary_is_parenthesized() { op: UnaryOp::Neg, expr: Box::new(bin(int(2), BinaryOp::Add, int(3))), }), - r#"SELECT - (2 + 3) FROM "users""# + r#"SELECT - (2 + 3)"# ); } @@ -2996,7 +2985,7 @@ fn collate_over_binary_is_parenthesized() { )), collation: "C".into(), }), - r#"SELECT ("users"."name" || "users"."department") COLLATE "C" FROM "users""# + r#"SELECT ("users"."name" || "users"."department") COLLATE "C""# ); } @@ -3012,7 +3001,7 @@ fn json_path_text_over_unary_not_is_parenthesized() { }), path: "k".into(), }), - r#"SELECT (NOT "users"."data")->>'k' FROM "users""# + r#"SELECT (NOT "users"."data")->>'k'"# ); } @@ -3026,7 +3015,7 @@ fn cast_over_nested_binary_parenthesizes_both_levels() { bin(bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, int(3)), "bigint", )), - r#"SELECT ((1 + 2) * 3)::bigint FROM "users""# + r#"SELECT ((1 + 2) * 3)::bigint"# ); } @@ -3040,7 +3029,7 @@ fn atomic_binary_operands_stay_bare() { BinaryOp::Add, Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), )), - r#"SELECT "users"."id" + abs("users"."age") FROM "users""# + r#"SELECT "users"."id" + abs("users"."age")"# ); } @@ -3068,3 +3057,63 @@ fn comparison_with_not_operand_is_parenthesized() { r#"SELECT * FROM "users" WHERE (NOT "users"."active") = $1"# ); } + +// ========================================================================== +// Operand positions missed by the first pass — a FieldRef with a JSON child +// renders an operator chain (`"data"->'age'`) just like Expr::JsonPathText, +// and the JSONB key operators feed their right operand into `?|` and `::text[]`. +// ========================================================================== + +fn json_field(table: &str, name: &str, child: &str) -> FieldRef { + let mut field = FieldDef::new(name); + field.child = Some(Box::new(FieldDef::new(child))); + FieldRef { + field, + table_name: table.into(), + namespace: None, + } +} + +#[test] +fn cast_over_field_with_json_child_is_parenthesized() { + // Bare `"users"."data"->'age'::bigint` is read as `"users"."data" -> ('age'::bigint)` + // — the same defect as Cast over JsonPathText, reached through a FieldRef. + assert_eq!( + cast_sql(Expr::Field(json_field("users", "data", "age")), "bigint"), + r#"SELECT ("users"."data"->'age')::bigint"# + ); +} + +#[test] +fn cast_over_field_without_json_child_stays_bare() { + assert_eq!( + cast_sql(Expr::Field(FieldRef::new("users", "age")), "bigint"), + r#"SELECT "users"."age"::bigint"# + ); +} + +#[test] +fn jsonb_has_any_key_parenthesizes_compound_right_operand() { + use qcraft_core::ast::expr::BinaryOp; + // `right` is the operand of both `?|` and the trailing `::text[]`. + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("users", "data")), + CompareOp::JsonbHasAnyKey, + bin( + Expr::Field(FieldRef::new("users", "a")), + BinaryOp::Concat, + Expr::Field(FieldRef::new("users", "b")), + ), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."data" ?| ("users"."a" || "users"."b")::text[]"# + ); +} diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index 484fc90..026c446 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -2446,48 +2446,48 @@ fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { } } -fn scalar_i64(expr: Expr) -> i64 { - let mut client = crate::test_client("template_dql"); - let stmt = QueryStmt { - columns: vec![SelectColumn::Expr { expr, alias: None }], - ..simple_query() +/// One database for the whole group: these are `SELECT ` with no FROM, so +/// they read no table and need no isolation — cloning a template per assertion +/// (what a per-test client would do) buys nothing. +#[test] +fn precedence_is_preserved_when_executed() { + let mut client = crate::test_client("template0"); + + let scalar_i64 = |client: &mut postgres::Client, expr: Expr| -> i64 { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(expr, "bigint"), + alias: None, + }], + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + client.query(&sql, ¶ms).unwrap()[0].get::<_, i64>(0) }; - let (sql, values) = render(&stmt); - let boxed = crate::common::to_pg_params(&values); - let params = crate::common::as_pg_params(&boxed); - let rows = client.query(&sql, ¶ms).unwrap(); - rows[0].get::<_, i64>(0) -} -#[test] -fn precedence_nested_binary_executes() { // (1 + 2) * 3 = 9; flat `1 + 2 * 3` silently yields 7. - let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); - assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), 9); -} + let nested = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); + assert_eq!(scalar_i64(&mut client, nested), 9, "nested binary"); -#[test] -fn precedence_right_operand_associativity_executes() { // 10 - (5 - 2) = 7; flat `10 - 5 - 2` is left-associative and yields 3. - let expr = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); - assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), 7); -} + let assoc = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); + assert_eq!( + scalar_i64(&mut client, assoc), + 7, + "right-operand associativity" + ); -#[test] -fn precedence_unary_over_binary_executes() { // -(2 + 3) = -5; flat `- 2 + 3` binds the minus to 2 and yields 1. - let expr = Expr::Unary { + let unary = Expr::Unary { op: UnaryOp::Neg, expr: Box::new(bin(lit(2), BinaryOp::Add, lit(3))), }; - assert_eq!(scalar_i64(Expr::cast(expr, "bigint")), -5); -} + assert_eq!(scalar_i64(&mut client, unary), -5, "unary over binary"); -#[test] -fn precedence_collate_over_binary_executes() { - // COLLATE binds tighter than ||, so a flat operand collates the integer 5 - // and PostgreSQL errors with "collations are not supported by type integer". - let mut client = crate::test_client("template_dql"); + // COLLATE binds tighter than ||, so a flat operand collates the integer 5 and + // PostgreSQL errors with "collations are not supported by type integer". let stmt = QueryStmt { columns: vec![SelectColumn::Expr { expr: Expr::Collate { @@ -2502,5 +2502,48 @@ fn precedence_collate_over_binary_executes() { let boxed = crate::common::to_pg_params(&values); let params = crate::common::as_pg_params(&boxed); let rows = client.query(&sql, ¶ms).unwrap(); - assert_eq!(rows[0].get::<_, String>(0), "x5"); + assert_eq!(rows[0].get::<_, String>(0), "x5", "collate over binary"); +} + +#[test] +fn cast_over_field_with_json_child_executes() { + // Same defect as cast-over-JsonPathText, reached through a FieldRef whose + // FieldDef carries a child: bare `"people"."data"->'age'::bigint` makes + // PostgreSQL cast the key literal and fail on `invalid input syntax`. + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "people" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "people" VALUES (1, '{"age": 36}'::jsonb)"#, + &[], + ) + .unwrap(); + + let mut field = FieldDef::new("data"); + field.child = Some(Box::new(FieldDef::new("age"))); + let field_ref = FieldRef { + field, + table_name: "people".into(), + namespace: None, + }; + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(Expr::Field(field_ref), "bigint"), + alias: Some("age".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("people"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows[0].get::<_, i64>(0), 36); } diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index 69b410e..c9fa858 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -922,9 +922,9 @@ impl Renderer for SqliteRenderer { } /// SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed - /// composite, so those two are already self-delimiting and must not collect a + /// composite, so those two already delimit themselves and must not collect a /// second pair of brackets. Every other operand follows the shared rule. - fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { + fn needs_operand_parens(&self, expr: &Expr) -> bool { if matches!( expr, Expr::Binary { @@ -932,15 +932,9 @@ impl Renderer for SqliteRenderer { .. } ) { - return self.render_expr(expr, ctx); + return false; } - if expr.needs_operand_parens() { - ctx.paren_open(); - self.render_expr(expr, ctx)?; - ctx.paren_close(); - return Ok(()); - } - self.render_expr(expr, ctx) + expr.needs_operand_parens() } fn render_compare_op( @@ -1033,7 +1027,7 @@ impl Renderer for SqliteRenderer { CompareOp::Regex => ctx.keyword("REGEXP"), CompareOp::IRegex => { ctx.keyword("REGEXP").string_literal("(?i)").keyword("||"); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; return Ok(()); } CompareOp::ILike => { diff --git a/crates/qcraft-sqlite/tests/dql.rs b/crates/qcraft-sqlite/tests/dql.rs index 6472a5e..18b334a 100644 --- a/crates/qcraft-sqlite/tests/dql.rs +++ b/crates/qcraft-sqlite/tests/dql.rs @@ -4,7 +4,7 @@ use qcraft_core::ast::custom::CustomBinaryOp; use qcraft_core::ast::expr::*; use qcraft_core::ast::query::*; use qcraft_core::ast::value::Value; -use qcraft_core::render::ctx::ParamStyle; +use qcraft_core::render::ctx::{ParamStyle, RenderCtx}; use qcraft_sqlite::SqliteRenderer; fn render(stmt: &QueryStmt) -> String { @@ -2074,14 +2074,7 @@ fn is_null_non_boolean_right_errors() { // for `||` vs `* /`, differ from PostgreSQL's). // ========================================================================== -fn sq_expr_sql(expr: Expr) -> String { - render(&QueryStmt { - columns: vec![SelectColumn::Expr { expr, alias: None }], - ..simple_query() - }) -} - -fn sq_bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { Expr::Binary { left: Box::new(left), op, @@ -2089,7 +2082,7 @@ fn sq_bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { } } -fn sq_int(n: i64) -> Expr { +fn int(n: i64) -> Expr { Expr::raw(n.to_string()) } @@ -2097,12 +2090,12 @@ fn sq_int(n: i64) -> Expr { fn nested_binary_left_operand_is_parenthesized() { // (1 + 2) * 3 — bare `1 + 2 * 3` would be 7, not 9. assert_eq!( - sq_expr_sql(sq_bin( - sq_bin(sq_int(1), BinaryOp::Add, sq_int(2)), + render_expr_sqlite(bin( + bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, - sq_int(3), + int(3), )), - r#"SELECT (1 + 2) * 3 FROM "users""# + r#"SELECT (1 + 2) * 3"# ); } @@ -2110,12 +2103,12 @@ fn nested_binary_left_operand_is_parenthesized() { fn nested_binary_right_operand_is_parenthesized() { // 10 - (5 - 2) — bare `10 - 5 - 2` is left-associative, giving 3, not 7. assert_eq!( - sq_expr_sql(sq_bin( - sq_int(10), + render_expr_sqlite(bin( + int(10), BinaryOp::Sub, - sq_bin(sq_int(5), BinaryOp::Sub, sq_int(2)), + bin(int(5), BinaryOp::Sub, int(2)), )), - r#"SELECT 10 - (5 - 2) FROM "users""# + r#"SELECT 10 - (5 - 2)"# ); } @@ -2123,40 +2116,40 @@ fn nested_binary_right_operand_is_parenthesized() { fn unary_over_binary_is_parenthesized() { // -(2 + 3) — bare `- 2 + 3` binds the minus to 2, giving 1, not -5. assert_eq!( - sq_expr_sql(Expr::Unary { + render_expr_sqlite(Expr::Unary { op: UnaryOp::Neg, - expr: Box::new(sq_bin(sq_int(2), BinaryOp::Add, sq_int(3))), + expr: Box::new(bin(int(2), BinaryOp::Add, int(3))), }), - r#"SELECT - (2 + 3) FROM "users""# + r#"SELECT - (2 + 3)"# ); } #[test] fn collate_over_binary_is_parenthesized() { assert_eq!( - sq_expr_sql(Expr::Collate { - expr: Box::new(sq_bin( + render_expr_sqlite(Expr::Collate { + expr: Box::new(bin( Expr::Field(FieldRef::new("users", "name")), BinaryOp::Concat, Expr::Field(FieldRef::new("users", "department")), )), collation: "NOCASE".into(), }), - r#"SELECT ("users"."name" || "users"."department") COLLATE NOCASE FROM "users""# + r#"SELECT ("users"."name" || "users"."department") COLLATE NOCASE"# ); } #[test] fn json_path_text_over_unary_not_is_parenthesized() { assert_eq!( - sq_expr_sql(Expr::JsonPathText { + render_expr_sqlite(Expr::JsonPathText { expr: Box::new(Expr::Unary { op: UnaryOp::Not, expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), }), path: "k".into(), }), - r#"SELECT (NOT "users"."data")->>'k' FROM "users""# + r#"SELECT (NOT "users"."data")->>'k'"# ); } @@ -2164,27 +2157,23 @@ fn json_path_text_over_unary_not_is_parenthesized() { fn cast_over_nested_binary_keeps_inner_parens() { // CAST(...) is self-delimiting, but the inner sum still needs its own parens. assert_eq!( - sq_expr_sql(Expr::cast( - sq_bin( - sq_bin(sq_int(1), BinaryOp::Add, sq_int(2)), - BinaryOp::Mul, - sq_int(3), - ), + render_expr_sqlite(Expr::cast( + bin(bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, int(3),), "INTEGER", )), - r#"SELECT CAST((1 + 2) * 3 AS INTEGER) FROM "users""# + r#"SELECT CAST((1 + 2) * 3 AS INTEGER)"# ); } #[test] fn atomic_binary_operands_stay_bare() { assert_eq!( - sq_expr_sql(sq_bin( + render_expr_sqlite(bin( Expr::Field(FieldRef::new("users", "id")), BinaryOp::Add, Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), )), - r#"SELECT "users"."id" + abs("users"."age") FROM "users""# + r#"SELECT "users"."id" + abs("users"."age")"# ); } @@ -2213,27 +2202,128 @@ fn comparison_with_not_operand_is_parenthesized() { fn cast_over_raw_stays_bare() { // Raw is an opaque escape hatch — never bracketed automatically. assert_eq!( - sq_expr_sql(Expr::cast(Expr::raw("a + b"), "INTEGER")), - r#"SELECT CAST(a + b AS INTEGER) FROM "users""# + render_expr_sqlite(Expr::cast(Expr::raw("a + b"), "INTEGER")), + r#"SELECT CAST(a + b AS INTEGER)"# ); } #[test] fn paren_operand_is_not_double_wrapped() { assert_eq!( - sq_expr_sql(sq_bin( - Expr::paren(sq_bin(sq_int(1), BinaryOp::Add, sq_int(2))), + render_expr_sqlite(bin( + Expr::paren(bin(int(1), BinaryOp::Add, int(2))), BinaryOp::Mul, - sq_int(3), + int(3), )), - r#"SELECT (1 + 2) * 3 FROM "users""# + r#"SELECT (1 + 2) * 3"# ); } #[test] fn paren_wraps_any_expression() { assert_eq!( - sq_expr_sql(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), - r#"SELECT ("users"."age") FROM "users""# + render_expr_sqlite(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), + r#"SELECT ("users"."age")"# + ); +} + +// ========================================================================== +// Operand positions missed by the first pass — a FieldRef with a JSON child +// renders an operator chain (`"data"->'name'`) just like Expr::JsonPathText, +// and IRegex feeds its right operand into a `||` concatenation. +// ========================================================================== + +fn json_field(table: &str, name: &str, child: &str) -> FieldRef { + let mut field = FieldDef::new(name); + field.child = Some(Box::new(FieldDef::new(child))); + FieldRef { + field, + table_name: table.into(), + namespace: None, + } +} + +#[test] +fn collate_over_field_with_json_child_is_parenthesized() { + // COLLATE binds tighter than `->`, so a bare operand collates the key literal. + assert_eq!( + render_expr_sqlite(Expr::Collate { + expr: Box::new(Expr::Field(json_field("users", "data", "name"))), + collation: "NOCASE".into(), + }), + r#"SELECT ("users"."data"->'name') COLLATE NOCASE"# + ); +} + +#[test] +fn collate_over_field_without_json_child_stays_bare() { + assert_eq!( + render_expr_sqlite(Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "name"))), + collation: "NOCASE".into(), + }), + r#"SELECT "users"."name" COLLATE NOCASE"# + ); +} + +#[test] +fn iregex_parenthesizes_compound_right_operand() { + // right is the RHS of `'(?i)' || right`. + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("users", "name")), + CompareOp::IRegex, + Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "pattern"))), + collation: "NOCASE".into(), + }, + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."name" REGEXP '(?i)' || ("users"."pattern" COLLATE NOCASE)"# + ); +} + +// ========================================================================== +// delegate_renderer! must forward render_operand / needs_operand_parens, or a +// wrapping renderer silently falls back to the trait default and loses the +// dialect's own rule (SQLite exempts power()/XOR, which delimit themselves). +// ========================================================================== + +struct WrappingRenderer { + inner: SqliteRenderer, +} + +impl qcraft_core::render::renderer::Renderer for WrappingRenderer { + qcraft_core::delegate_renderer!(self.inner); +} + +#[test] +fn wrapping_renderer_delegates_render_operand() { + use qcraft_core::render::renderer::Renderer; + + let expr = bin( + Expr::Field(FieldRef::new("t", "a")), + BinaryOp::Power, + Expr::Field(FieldRef::new("t", "b")), ); + + let mut wrapped = RenderCtx::new(ParamStyle::QMark); + WrappingRenderer { + inner: SqliteRenderer::new(), + } + .render_operand(&expr, &mut wrapped) + .unwrap(); + + let mut direct = RenderCtx::new(ParamStyle::QMark); + SqliteRenderer::new() + .render_operand(&expr, &mut direct) + .unwrap(); + + assert_eq!(wrapped.sql(), direct.sql()); + assert_eq!(wrapped.sql(), r#"power("t"."a", "t"."b")"#); } diff --git a/crates/qcraft-sqlite/tests/integration_dql.rs b/crates/qcraft-sqlite/tests/integration_dql.rs index 38bc4ff..87b4701 100644 --- a/crates/qcraft-sqlite/tests/integration_dql.rs +++ b/crates/qcraft-sqlite/tests/integration_dql.rs @@ -2259,8 +2259,8 @@ fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { } fn scalar(expr: Expr) -> T { + // These queries are `SELECT ` with no FROM — no seeded tables needed. let conn = Connection::open_in_memory().unwrap(); - setup_db(&conn); let stmt = QueryStmt { columns: vec![SelectColumn::Expr { expr, alias: None }], from: None, From f50449a6ada02a5d3b6ea5ffbd494f7b7df6a234 Mon Sep 17 00:00:00 2001 From: Emil Temirov Date: Tue, 14 Jul 2026 20:46:02 +0300 Subject: [PATCH 3/4] fix: drop Expr::Paren, keep Raw grouping in the caller's hands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 9 ++-- crates/qcraft-core/src/ast/expr.rs | 24 +++------- crates/qcraft-core/src/render/renderer.rs | 7 --- crates/qcraft-postgres/src/lib.rs | 7 --- crates/qcraft-postgres/tests/dql.rs | 46 ++++--------------- .../qcraft-postgres/tests/integration/dql.rs | 42 +++++++++++++++++ crates/qcraft-sqlite/src/lib.rs | 7 --- crates/qcraft-sqlite/tests/dql.rs | 20 -------- docs/type-reference.md | 19 ++++---- 9 files changed, 69 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 678bb56..a90762f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,20 +12,17 @@ Two further operand positions were missed on the first pass and are now covered: the right operand of PostgreSQL's `?|` / `?&` (which also feeds a trailing `::text[]`), and the right operand of SQLite's `IRegex` (the RHS of a `||` concatenation). ### Added -- `Expr::Paren(Box)` and the `Expr::paren(expr)` constructor — explicit grouping. Operator operands are bracketed automatically, so this is only needed to group an opaque `Raw` / `Custom` expression or to force brackets for readability. - `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. - `Renderer::needs_operand_parens()` — the extension point a dialect overrides when its own rendering of a node already delimits it (SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed composite, so neither takes a second pair of brackets). Overriding the predicate rather than `render_operand` keeps the bracketing logic single-sourced. - -### Fixed (internal) -- `delegate_renderer!` now forwards `render_operand` and `needs_operand_parens`. Without those arms a wrapping renderer silently fell back to the trait default and lost the inner dialect's rule. +- `delegate_renderer!` forwards `needs_operand_parens`, so a wrapping renderer keeps the inner dialect's rule instead of falling back to the core default. ### Changed - Generated SQL now carries brackets where operand structure demands them (values are unchanged; only the SQL text differs). Snapshot tests that compare rendered SQL byte-for-byte may need updating — for example a `COLLATE` operand in a comparison now renders as `("users"."name" COLLATE "C") = $1`. - Unary operators render via `keyword()` instead of `write()`, so `SELECT- x` is now `SELECT - x`. ### Notes -- `Expr::Raw` and `Expr::Custom` are **never** bracketed automatically: their contents are opaque and need not be an expression at all. Wrap them in `Expr::Paren` when they need grouping — e.g. `Expr::cast(Expr::paren(Expr::raw("a + b")), "text")` renders `(a + b)::text`, while the unwrapped form renders `a + b::text`. -- Adding the `Expr::Paren` variant is source-breaking for exhaustive `match` over `Expr`. +- `Expr::Raw` and `Expr::Custom` are **never** bracketed automatically: their contents are opaque and need not be an expression at all. A caller who needs grouping writes it into the fragment itself — `Expr::cast(Expr::raw("(price * qty)"), "numeric")` renders `(price * qty)::numeric`, while `Expr::raw("price * qty")` renders `price * qty::numeric` (the cast lands on `qty`). +- No new `Expr` variant is added, so exhaustive `match` over `Expr` still compiles. ## 3.1.0 diff --git a/crates/qcraft-core/src/ast/expr.rs b/crates/qcraft-core/src/ast/expr.rs index 07cfaeb..ade8948 100644 --- a/crates/qcraft-core/src/ast/expr.rs +++ b/crates/qcraft-core/src/ast/expr.rs @@ -50,13 +50,6 @@ pub enum Expr { /// Collation override: `expr COLLATE "name"`. Collate { expr: Box, collation: String }, - /// Explicit grouping: `(expr)`. - /// - /// Operator operands are bracketed automatically (see [`Expr::needs_operand_parens`]), - /// so this is only needed to group an opaque `Raw`/`Custom` expression or to force - /// brackets for readability. - Paren(Box), - /// Build a JSON array: PG `jsonb_build_array(...)`, SQLite `json_array(...)`. JsonArray(Vec), @@ -280,11 +273,6 @@ impl Expr { Expr::Now } - /// Explicit grouping: `(expr)`. - pub fn paren(expr: Expr) -> Self { - Expr::Paren(Box::new(expr)) - } - /// True if this expression must be parenthesized when it appears as the operand /// of an operator (`+`, `::`, `COLLATE`, `->>`, a comparison, …). /// @@ -301,10 +289,12 @@ impl Expr { /// is a bare identifier and is not. /// /// Self-delimiting forms (literals, identifiers, function calls, `CAST(…)`, - /// `CASE … END`, subqueries, tuples, [`Expr::Paren`]) carry their own boundaries - /// and render bare. `Raw` and `Custom` are opaque escape hatches whose contents - /// need not be an expression at all, so they are never bracketed automatically — - /// wrap them in [`Expr::Paren`] when they need grouping. + /// `CASE … END`, subqueries, tuples) carry their own boundaries and render bare. + /// + /// `Raw` and `Custom` are opaque escape hatches whose contents need not be an + /// expression at all, so they are never bracketed automatically. A caller who needs + /// grouping writes it into the fragment itself — `Expr::raw("(price * qty)")` — and + /// a `CustomExpr` author controls their own rendering. pub fn needs_operand_parens(&self) -> bool { match self { Expr::Binary { .. } @@ -333,7 +323,6 @@ impl Expr { | Expr::Cast { expr, .. } | Expr::Collate { expr, .. } | Expr::JsonPathText { expr, .. } => expr.contains_unbound_param(), - Expr::Paren(expr) => expr.contains_unbound_param(), Expr::Func { args, .. } | Expr::Tuple(args) | Expr::JsonArray(args) => { args.iter().any(|a| a.contains_unbound_param()) } @@ -410,7 +399,6 @@ impl Expr { | Expr::Cast { expr, .. } | Expr::Collate { expr, .. } | Expr::JsonPathText { expr, .. } => expr.contains_subquery(), - Expr::Paren(expr) => expr.contains_subquery(), Expr::Func { args, .. } | Expr::Tuple(args) | Expr::JsonArray(args) => { args.iter().any(|a| a.contains_subquery()) } diff --git a/crates/qcraft-core/src/render/renderer.rs b/crates/qcraft-core/src/render/renderer.rs index ac550e5..f2bb22b 100644 --- a/crates/qcraft-core/src/render/renderer.rs +++ b/crates/qcraft-core/src/render/renderer.rs @@ -205,13 +205,6 @@ macro_rules! delegate_renderer { fn needs_operand_parens(&$self, expr: &$crate::ast::expr::Expr) -> bool { $self.$inner.needs_operand_parens(expr) } - fn render_operand( - &$self, - expr: &$crate::ast::expr::Expr, - ctx: &mut $crate::render::ctx::RenderCtx, - ) -> $crate::error::RenderResult<()> { - $self.$inner.render_operand(expr, ctx) - } fn render_aggregate( &$self, agg: &$crate::ast::expr::AggregationDef, diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index 51dcf19..e188fba 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -1010,13 +1010,6 @@ impl Renderer for PostgresRenderer { Ok(()) } - Expr::Paren(inner) => { - ctx.paren_open(); - self.render_expr(inner, ctx)?; - ctx.paren_close(); - Ok(()) - } - Expr::JsonArray(items) => { ctx.keyword("jsonb_build_array").write("("); for (i, item) in items.iter().enumerate() { diff --git a/crates/qcraft-postgres/tests/dql.rs b/crates/qcraft-postgres/tests/dql.rs index c5db5b4..6ea43d3 100644 --- a/crates/qcraft-postgres/tests/dql.rs +++ b/crates/qcraft-postgres/tests/dql.rs @@ -2827,45 +2827,15 @@ fn cast_over_collate_is_parenthesized() { #[test] fn cast_over_raw_stays_bare() { - // Raw is an opaque escape hatch: its contents may not even be an expression, - // so the renderer never brackets it. Callers who need grouping wrap it in - // Expr::Paren themselves. + // Raw is an opaque escape hatch: its contents may not even be an expression, so the + // renderer never brackets it. A caller who needs grouping writes it into the + // fragment: Expr::raw("(a + b)"). assert_eq!( cast_sql(Expr::raw("a + b"), "text"), r#"SELECT a + b::text"# ); } -#[test] -fn cast_over_parenthesized_raw_is_grouped() { - assert_eq!( - cast_sql(Expr::paren(Expr::raw("a + b")), "text"), - r#"SELECT (a + b)::text"# - ); -} - -#[test] -fn paren_wraps_any_expression() { - assert_eq!( - expr_sql(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), - r#"SELECT ("users"."age")"# - ); -} - -#[test] -fn paren_operand_is_not_double_wrapped() { - use qcraft_core::ast::expr::BinaryOp; - // Paren is self-delimiting: it must not attract a second pair of brackets. - assert_eq!( - expr_sql(bin( - Expr::paren(bin(int(1), BinaryOp::Add, int(2))), - BinaryOp::Mul, - int(3), - )), - r#"SELECT (1 + 2) * 3"# - ); -} - #[test] fn cast_over_field_stays_bare() { assert_eq!( @@ -3095,7 +3065,9 @@ fn cast_over_field_without_json_child_stays_bare() { #[test] fn jsonb_has_any_key_parenthesizes_compound_right_operand() { use qcraft_core::ast::expr::BinaryOp; - // `right` is the operand of both `?|` and the trailing `::text[]`. + // `right` is the operand of both `?|` and the trailing `::text[]`, so a compound + // operand must be bracketed. `||` on two arrays concatenates them, which is the + // only compound shape that is meaningful here (a text concat cannot cast to text[]). let stmt = QueryStmt { columns: vec![SelectColumn::all()], from: Some(users_from()), @@ -3104,9 +3076,9 @@ fn jsonb_has_any_key_parenthesizes_compound_right_operand() { Expr::Field(FieldRef::new("users", "data")), CompareOp::JsonbHasAnyKey, bin( - Expr::Field(FieldRef::new("users", "a")), + Expr::raw("ARRAY['email']"), BinaryOp::Concat, - Expr::Field(FieldRef::new("users", "b")), + Expr::raw("ARRAY['phone']"), ), ), ))])), @@ -3114,6 +3086,6 @@ fn jsonb_has_any_key_parenthesizes_compound_right_operand() { }; assert_eq!( render(&stmt), - r#"SELECT * FROM "users" WHERE "users"."data" ?| ("users"."a" || "users"."b")::text[]"# + r#"SELECT * FROM "users" WHERE "users"."data" ?| (ARRAY['email'] || ARRAY['phone'])::text[]"# ); } diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index 026c446..b01bb30 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -2547,3 +2547,45 @@ fn cast_over_field_with_json_child_executes() { let rows = client.query(&sql, ¶ms).unwrap(); assert_eq!(rows[0].get::<_, i64>(0), 36); } + +#[test] +fn jsonb_has_any_key_with_compound_operand_executes() { + // The `?|` right operand carries a renderer-owned `::text[]`, so a compound operand + // must be bracketed or the cast lands on the wrong sub-expression. + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "docs" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "docs" VALUES (1, '{"phone": "555"}'::jsonb)"#, + &[], + ) + .unwrap(); + + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(vec![FromItem::table(SchemaRef::new("docs"))]), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("docs", "data")), + CompareOp::JsonbHasAnyKey, + bin( + Expr::raw("ARRAY['email']"), + BinaryOp::Concat, + Expr::raw("ARRAY['phone']"), + ), + ), + ))])), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1, "row has key 'phone', so ?| must match"); +} diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index c9fa858..91f23bf 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -651,13 +651,6 @@ impl Renderer for SqliteRenderer { Ok(()) } - Expr::Paren(inner) => { - ctx.paren_open(); - self.render_expr(inner, ctx)?; - ctx.paren_close(); - Ok(()) - } - Expr::JsonArray(items) => { ctx.keyword("json_array").write("("); for (i, item) in items.iter().enumerate() { diff --git a/crates/qcraft-sqlite/tests/dql.rs b/crates/qcraft-sqlite/tests/dql.rs index 18b334a..61d6f21 100644 --- a/crates/qcraft-sqlite/tests/dql.rs +++ b/crates/qcraft-sqlite/tests/dql.rs @@ -2207,26 +2207,6 @@ fn cast_over_raw_stays_bare() { ); } -#[test] -fn paren_operand_is_not_double_wrapped() { - assert_eq!( - render_expr_sqlite(bin( - Expr::paren(bin(int(1), BinaryOp::Add, int(2))), - BinaryOp::Mul, - int(3), - )), - r#"SELECT (1 + 2) * 3"# - ); -} - -#[test] -fn paren_wraps_any_expression() { - assert_eq!( - render_expr_sqlite(Expr::paren(Expr::Field(FieldRef::new("users", "age")))), - r#"SELECT ("users"."age")"# - ); -} - // ========================================================================== // Operand positions missed by the first pass — a FieldRef with a JSON child // renders an operator chain (`"data"->'name'`) just like Expr::JsonPathText, diff --git a/docs/type-reference.md b/docs/type-reference.md index 6186a87..9192a0f 100644 --- a/docs/type-reference.md +++ b/docs/type-reference.md @@ -83,7 +83,6 @@ pub enum Expr { SubQuery(Box), ArraySubQuery(Box), Collate { expr: Box, collation: String }, - Paren(Box), Raw { sql: String, params: Vec }, JsonArray(Vec), JsonObject(Vec<(String, Expr)>), @@ -112,7 +111,6 @@ pub enum Expr { | `Expr::exists(query)` | `Expr::Exists(Box::new(query))` | | `Expr::subquery(query)` | `Expr::SubQuery(Box::new(query))` | | `expr.collate("C")` | `Expr::Collate { expr, collation: "C" }` | -| `Expr::paren(expr)` | `Expr::Paren(Box::new(expr))` — explicit grouping | | `Expr::json_array(vec![...])` | `Expr::JsonArray(...)` — PG: `jsonb_build_array`, SQLite: `json_array` | | `Expr::json_object(vec![...])` | `Expr::JsonObject(...)` — PG: `jsonb_build_object`, SQLite: `json_object` | | `Expr::json_agg(expr)` | `Expr::JsonAgg { ... }` — PG: `jsonb_agg`, SQLite: `json_group_array` | @@ -144,18 +142,19 @@ Expr::Binary { // PG and SQLite: (1 + 2) * 3 ``` -Operands that are `Binary`, `Unary`, `Collate`, `JsonPathText` or `Window` get brackets; -self-delimiting forms (literals, fields, function calls, `CAST(…)`, `CASE … END`, -subqueries, tuples, `Paren`) render bare. Bracketing is structural rather than driven by a -precedence table, because precedence differs per dialect — SQLite binds `||` tighter than -`*`, PostgreSQL binds it looser than `+`. +Operands that are `Binary`, `Unary`, `Collate`, `JsonPathText`, `Window`, or a `Field` +whose `FieldDef` carries a JSON child get brackets; self-delimiting forms (literals, +fields, function calls, `CAST(…)`, `CASE … END`, subqueries, tuples) render bare. +Bracketing is structural rather than driven by a precedence table, because precedence +differs per dialect — 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. Wrap them yourself when they need grouping: +need not be an expression at all. Write the grouping into the fragment itself: ```rust -Expr::cast(Expr::raw("a + b"), "text") // a + b::text ← cast binds to b -Expr::cast(Expr::paren(Expr::raw("a + b")), "text") // (a + b)::text +Expr::cast(Expr::raw("price * qty"), "numeric") // price * qty::numeric ← cast binds to qty +Expr::cast(Expr::raw("(price * qty)"), "numeric") // (price * qty)::numeric ``` ## FieldRef / FieldDef From b73c20553dc4fe4474cbc33251960c3083346127 Mon Sep 17 00:00:00 2001 From: Emil Temirov Date: Tue, 14 Jul 2026 22:04:37 +0300 Subject: [PATCH 4/4] feat: Custom* nodes render themselves, so extensions actually work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 6 +- crates/qcraft-core/src/ast/custom.rs | 61 +++- crates/qcraft-core/src/ast/expr.rs | 2 + crates/qcraft-core/src/render/renderer.rs | 23 +- crates/qcraft-postgres/src/lib.rs | 37 +-- crates/qcraft-postgres/tests/extensibility.rs | 265 ++++++++++++++++++ .../qcraft-postgres/tests/integration/dql.rs | 82 ++++++ crates/qcraft-sqlite/src/lib.rs | 21 +- docs/extensibility.md | 82 +++--- 9 files changed, 493 insertions(+), 86 deletions(-) create mode 100644 crates/qcraft-postgres/tests/extensibility.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index a90762f..f4bfbfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,11 @@ Two further operand positions were missed on the first pass and are now covered: the right operand of PostgreSQL's `?|` / `?&` (which also feeds a trailing `::text[]`), and the right operand of SQLite's `IRegex` (the RHS of a `||` concatenation). ### Added -- `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. +- **`Custom*` nodes now render themselves.** Every `Custom*` trait gained `render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx)`, and `CustomExpr` also gained `needs_operand_parens()`. The node is handed the renderer, so it recurses back through it for its own sub-expressions — nested expressions, identifier quoting and parameter numbering all go through the same dialect and the same `RenderCtx`. Both default to an error / `true`, so nothing existing breaks and an unrenderable node still fails loudly. + + This makes the extension story actually work. Previously `Expr::Custom` and `ConditionNode::Custom` returned "must be handled by a wrapping renderer", but such a wrapper could not be written: `delegate_renderer!` emits every trait method, so combining it with an override is a duplicate definition (`E0201`), and even a hand-written wrapper never saw the call — a dialect's `render_query` recurses through its own concrete `self`, never back out to the wrapper. Teaching qcraft a new syntax now needs no wrapper at all: implement `render` on the node. qcraft's own `PgVectorOp` (`<->`, `<#>`, …) is now implemented exactly this way, through the same public mechanism users get. + +- `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. A `CustomExpr` author calls `renderer.render_operand()` for its own operands and answers `needs_operand_parens()` for itself, so a user-defined infix node (`x AT TIME ZONE 'UTC'`) is bracketed correctly under a cast. - `Renderer::needs_operand_parens()` — the extension point a dialect overrides when its own rendering of a node already delimits it (SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed composite, so neither takes a second pair of brackets). Overriding the predicate rather than `render_operand` keeps the bracketing logic single-sourced. - `delegate_renderer!` forwards `needs_operand_parens`, so a wrapping renderer keeps the inner dialect's rule instead of falling back to the core default. diff --git a/crates/qcraft-core/src/ast/custom.rs b/crates/qcraft-core/src/ast/custom.rs index 687449c..4d9ea4b 100644 --- a/crates/qcraft-core/src/ast/custom.rs +++ b/crates/qcraft-core/src/ast/custom.rs @@ -1,6 +1,10 @@ use std::any::Any; use std::fmt::Debug; +use crate::error::{RenderError, RenderResult}; +use crate::render::ctx::RenderCtx; +use crate::render::renderer::Renderer; + // --------------------------------------------------------------------------- // Macro to define Custom* traits with common boilerplate // --------------------------------------------------------------------------- @@ -10,6 +14,25 @@ macro_rules! define_custom_trait { pub trait $trait_name: Debug + Send + Sync { fn as_any(&self) -> &dyn Any; fn clone_box(&self) -> Box; + + /// Render this node. The extension lives in the node itself, not in a renderer + /// wrapped around the dialect: rendering recurses back through `renderer`, so + /// nested expressions, identifier quoting and parameter numbering all go + /// through the same dialect and the same [`RenderCtx`] as everything else. + /// + /// The default is an error, keeping the contract that an unrenderable custom + /// node fails loudly rather than silently dropping SQL. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + let _ = (renderer, ctx); + Err(RenderError::unsupported( + stringify!($trait_name), + concat!( + "implement ", + stringify!($trait_name), + "::render to teach the renderer this node" + ), + )) + } } impl Clone for Box { @@ -20,7 +43,6 @@ macro_rules! define_custom_trait { }; } -define_custom_trait!(CustomExpr); define_custom_trait!(CustomCondition); define_custom_trait!(CustomCompareOp); define_custom_trait!(CustomTableSource); @@ -30,3 +52,40 @@ define_custom_trait!(CustomFieldType); define_custom_trait!(CustomBinaryOp); define_custom_trait!(CustomConstraint); define_custom_trait!(CustomTransaction); + +/// A user-defined expression node. +/// +/// Defined by hand rather than through `define_custom_trait!` because, unlike the other +/// custom nodes, an expression can appear as the operand of an operator and therefore has +/// to answer whether it needs brackets there. +pub trait CustomExpr: Debug + Send + Sync { + fn as_any(&self) -> &dyn Any; + fn clone_box(&self) -> Box; + + /// Render this node. See [`CustomCondition::render`] — same contract: the node renders + /// itself and recurses back through `renderer` for any sub-expression it holds. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + let _ = (renderer, ctx); + Err(RenderError::unsupported( + "CustomExpr", + "implement CustomExpr::render to teach the renderer this node", + )) + } + + /// Whether this node needs brackets when it is the operand of an operator + /// (`+`, `::`, `COLLATE`, a comparison, …). Only the author knows the shape it renders. + /// + /// Defaults to `true` — conservative, because a node rendering an infix form + /// (`x AT TIME ZONE 'UTC'`) would otherwise be re-associated by the engine's + /// precedence, which is the exact class of bug operand bracketing exists to prevent. + /// A node rendering a self-delimiting form (`my_func(x)`) should return `false`. + fn needs_operand_parens(&self) -> bool { + true + } +} + +impl Clone for Box { + fn clone(&self) -> Self { + self.clone_box() + } +} diff --git a/crates/qcraft-core/src/ast/expr.rs b/crates/qcraft-core/src/ast/expr.rs index ade8948..39a0e34 100644 --- a/crates/qcraft-core/src/ast/expr.rs +++ b/crates/qcraft-core/src/ast/expr.rs @@ -303,6 +303,8 @@ impl Expr { | Expr::JsonPathText { .. } | Expr::Window(_) => true, Expr::Field(field_ref) => field_ref.field.child.is_some(), + // Only the node's author knows the shape it renders. + Expr::Custom(custom) => custom.needs_operand_parens(), _ => false, } } diff --git a/crates/qcraft-core/src/render/renderer.rs b/crates/qcraft-core/src/render/renderer.rs index f2bb22b..c9b78ad 100644 --- a/crates/qcraft-core/src/render/renderer.rs +++ b/crates/qcraft-core/src/render/renderer.rs @@ -98,14 +98,25 @@ pub trait Renderer { fn render_index_def(&self, idx: &IndexDef, ctx: &mut RenderCtx) -> RenderResult<()>; } -/// Macro to delegate all Renderer methods to an inner renderer. +/// Delegates every [`Renderer`] method to an inner renderer. +/// +/// The macro emits **all** methods, so it cannot be combined with overriding one — +/// that is a duplicate definition (`error[E0201]`). It is for wrapping a dialect +/// wholesale, not for extending it. +/// +/// To teach the renderer a syntax it does not know, do **not** wrap the renderer: +/// implement [`CustomExpr::render`](crate::ast::custom::CustomExpr::render) on the node +/// itself. The node is handed the renderer and recurses back through it, so nested +/// expressions, quoting and parameter numbering stay consistent: /// -/// Usage: /// ```ignore -/// struct MyRenderer { inner: PostgresRenderer } -/// impl Renderer for MyRenderer { -/// fn render_cast(&self, ...) { /* custom */ } -/// delegate_renderer!(self.inner); +/// impl CustomExpr for AtTimeZone { +/// fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { +/// renderer.render_operand(&self.expr, ctx)?; // brackets the operand if needed +/// ctx.keyword("AT TIME ZONE").string_literal(&self.zone); +/// Ok(()) +/// } +/// fn needs_operand_parens(&self) -> bool { true } // infix — bracket me as an operand /// } /// ``` #[macro_export] diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index e188fba..2209931 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -51,28 +51,21 @@ impl CustomBinaryOp for PgVectorOp { fn clone_box(&self) -> Box { Box::new(*self) } -} -impl From for BinaryOp { - fn from(op: PgVectorOp) -> Self { - BinaryOp::Custom(Box::new(op)) - } -} - -fn render_custom_binary_op(custom: &dyn CustomBinaryOp, ctx: &mut RenderCtx) -> RenderResult<()> { - if let Some(op) = custom.as_any().downcast_ref::() { - ctx.write(match op { + fn render(&self, _renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + ctx.write(match self { PgVectorOp::L2Distance => " <-> ", PgVectorOp::InnerProduct => " <#> ", PgVectorOp::CosineDistance => " <=> ", PgVectorOp::L1Distance => " <+> ", }); Ok(()) - } else { - Err(RenderError::unsupported( - "CustomBinaryOp", - "unknown custom binary operator; use a wrapping renderer to handle it", - )) + } +} + +impl From for BinaryOp { + fn from(op: PgVectorOp) -> Self { + BinaryOp::Custom(Box::new(op)) } } @@ -923,7 +916,7 @@ impl Renderer for PostgresRenderer { }; match op { BinaryOp::Custom(custom) => { - render_custom_binary_op(custom.as_ref(), ctx)?; + custom.render(self, ctx)?; } _ => { ctx.keyword(match op { @@ -1141,10 +1134,7 @@ impl Renderer for PostgresRenderer { Ok(()) } - Expr::Custom(_) => Err(RenderError::unsupported( - "CustomExpr", - "custom expression must be handled by a wrapping renderer", - )), + Expr::Custom(custom) => custom.render(self, ctx), } } @@ -1263,11 +1253,8 @@ impl Renderer for PostgresRenderer { self.render_query(query, ctx)?; ctx.paren_close(); } - ConditionNode::Custom(_) => { - return Err(RenderError::unsupported( - "CustomCondition", - "custom condition must be handled by a wrapping renderer", - )); + ConditionNode::Custom(custom) => { + custom.render(self, ctx)?; } } } diff --git a/crates/qcraft-postgres/tests/extensibility.rs b/crates/qcraft-postgres/tests/extensibility.rs new file mode 100644 index 0000000..6e87469 --- /dev/null +++ b/crates/qcraft-postgres/tests/extensibility.rs @@ -0,0 +1,265 @@ +//! Extensibility: a user teaches qcraft a syntax it does not know, without waiting +//! for a release. The node itself knows how to render; it is handed the renderer so +//! it can recurse into its own sub-expressions and bind parameters through the same +//! context as everything else. + +use std::any::Any; +use std::fmt; + +use qcraft_core::ast::common::{FieldRef, SchemaRef}; +use qcraft_core::ast::conditions::*; +use qcraft_core::ast::custom::{CustomBinaryOp, CustomCondition, CustomExpr}; +use qcraft_core::ast::expr::*; +use qcraft_core::ast::query::*; +use qcraft_core::ast::value::Value; +use qcraft_core::error::RenderResult; +use qcraft_core::render::ctx::RenderCtx; +use qcraft_core::render::renderer::Renderer; +use qcraft_postgres::PostgresRenderer; + +// ── A user-defined expression: ` AT TIME ZONE ''` ──────────────── +// Infix, so it is NOT self-delimiting: as the operand of `::` it must be bracketed. + +#[derive(Clone)] +struct AtTimeZone { + expr: Expr, + zone: String, +} + +impl fmt::Debug for AtTimeZone { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "AtTimeZone") + } +} + +impl CustomExpr for AtTimeZone { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + // AT TIME ZONE is an operator, so its own left-hand side is an operand: + // `a + b AT TIME ZONE 'UTC'` would bind the zone to `b`. render_operand + // brackets it when the sub-expression's structure requires it. + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) + } + + fn needs_operand_parens(&self) -> bool { + true + } +} + +// ── A user-defined binary operator: `<->` style, but theirs ────────────────── + +#[derive(Debug, Clone)] +struct SameDay; + +impl CustomBinaryOp for SameDay { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render(&self, _renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + ctx.write(" <=> "); + Ok(()) + } +} + +// ── A user-defined condition: ` IS DISTINCT FROM ` ───────────── + +#[derive(Clone)] +struct IsDistinctFrom { + field: FieldRef, + value: Value, +} + +impl fmt::Debug for IsDistinctFrom { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "IsDistinctFrom") + } +} + +impl CustomCondition for IsDistinctFrom { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + renderer.render_expr(&Expr::Field(self.field.clone()), ctx)?; + ctx.keyword("IS DISTINCT FROM"); + ctx.param(self.value.clone()); + Ok(()) + } +} + +fn render(stmt: &QueryStmt) -> (String, Vec) { + PostgresRenderer::new().render_query_stmt(stmt).unwrap() +} + +fn base() -> QueryStmt { + QueryStmt { + ctes: None, + columns: vec![], + distinct: None, + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + joins: None, + where_clause: None, + group_by: None, + having: None, + window: None, + order_by: None, + limit: None, + lock: None, + set_op: None, + } +} + +fn at_utc(table: &str, col: &str) -> Expr { + Expr::Custom(Box::new(AtTimeZone { + expr: Expr::field(table, col), + zone: "UTC".into(), + })) +} + +#[test] +fn custom_expr_renders_in_a_real_query() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: at_utc("events", "created_at"), + alias: Some("created_utc".into()), + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!( + sql, + r#"SELECT "events"."created_at" AT TIME ZONE 'UTC' AS "created_utc" FROM "events""# + ); +} + +#[test] +fn custom_expr_as_cast_operand_is_bracketed() { + // `::` binds tighter than AT TIME ZONE, so a bare operand would cast the zone + // literal: `created_at AT TIME ZONE ('UTC'::date)`. The node says it is infix. + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(at_utc("events", "created_at"), "date"), + alias: None, + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!( + sql, + r#"SELECT ("events"."created_at" AT TIME ZONE 'UTC')::date FROM "events""# + ); +} + +#[test] +fn custom_expr_recurses_through_the_renderer_and_binds_params() { + // The custom node holds a sub-expression containing a parameter. Rendering must go + // back through the renderer so the placeholder is numbered in document order. + let inner = Expr::Custom(Box::new(AtTimeZone { + expr: Expr::Binary { + left: Box::new(Expr::field("events", "created_at")), + op: BinaryOp::Add, + right: Box::new(Expr::Value(Value::Int(7))), + }, + zone: "UTC".into(), + })); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: inner, + alias: None, + }], + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::field("events", "id"), + CompareOp::Eq, + Expr::Value(Value::Int(42)), + ), + ))])), + ..base() + }; + let (sql, params) = render(&stmt); + assert_eq!( + sql, + r#"SELECT ("events"."created_at" + $1) AT TIME ZONE 'UTC' FROM "events" WHERE "events"."id" = $2"# + ); + assert_eq!(params, vec![Value::Int(7), Value::Int(42)]); +} + +#[test] +fn custom_binary_op_renders() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Binary { + left: Box::new(Expr::field("events", "a")), + op: BinaryOp::Custom(Box::new(SameDay)), + right: Box::new(Expr::field("events", "b")), + }, + alias: None, + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!(sql, r#"SELECT "events"."a" <=> "events"."b" FROM "events""#); +} + +#[test] +fn custom_condition_renders_and_binds_params() { + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + where_clause: Some(Conditions::and(vec![ConditionNode::Custom(Box::new( + IsDistinctFrom { + field: FieldRef::new("events", "status"), + value: Value::Str("done".into()), + }, + ))])), + ..base() + }; + let (sql, params) = render(&stmt); + assert_eq!( + sql, + r#"SELECT * FROM "events" WHERE "events"."status" IS DISTINCT FROM $1"# + ); + assert_eq!(params, vec![Value::Str("done".into())]); +} + +#[test] +fn a_custom_node_that_does_not_implement_render_still_errors() { + // The default keeps the old contract: an unrenderable custom node is a clear error, + // not silently dropped SQL. + #[derive(Debug, Clone)] + struct Unrenderable; + impl CustomExpr for Unrenderable { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + } + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Custom(Box::new(Unrenderable)), + alias: None, + }], + ..base() + }; + let err = PostgresRenderer::new() + .render_query_stmt(&stmt) + .unwrap_err() + .to_string(); + assert!(err.contains("CustomExpr"), "unexpected error: {err}"); +} diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index b01bb30..9ea32e7 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -2589,3 +2589,85 @@ fn jsonb_has_any_key_with_compound_operand_executes() { let rows = client.query(&sql, ¶ms).unwrap(); assert_eq!(rows.len(), 1, "row has key 'phone', so ?| must match"); } + +// ========================================================================== +// Extensibility — a user-defined expression must not just render, it must run. +// ========================================================================== + +#[derive(Clone)] +struct AtTimeZone { + expr: Expr, + zone: String, +} + +impl std::fmt::Debug for AtTimeZone { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "AtTimeZone") + } +} + +impl qcraft_core::ast::custom::CustomExpr for AtTimeZone { + fn as_any(&self) -> &dyn std::any::Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render( + &self, + renderer: &dyn qcraft_core::render::renderer::Renderer, + ctx: &mut qcraft_core::render::ctx::RenderCtx, + ) -> qcraft_core::error::RenderResult<()> { + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) + } +} + +#[test] +fn user_defined_expression_executes_against_postgres() { + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "events" ("id" INTEGER PRIMARY KEY, "created_at" TIMESTAMPTZ NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "events" VALUES (1, '2024-03-01 12:00:00+00')"#, + &[], + ) + .unwrap(); + + // AT TIME ZONE is syntax qcraft does not know. The user teaches it, and it must + // survive a cast — `::` binds tighter than AT TIME ZONE, so without brackets PG + // reads `created_at AT TIME ZONE ('UTC'::text)` and rejects the statement. + let expr = Expr::cast( + Expr::Custom(Box::new(AtTimeZone { + expr: Expr::field("events", "created_at"), + zone: "UTC".into(), + })), + "text", + ); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr, + alias: Some("day".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + assert!(sql.contains("AT TIME ZONE"), "sql: {sql}"); + + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert!( + rows[0].get::<_, String>(0).starts_with("2024-03-01"), + "got: {}", + rows[0].get::<_, String>(0) + ); +} diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index 91f23bf..86ef8b9 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -499,10 +499,11 @@ impl Renderer for SqliteRenderer { } Expr::Binary { left, op, right } => match op { - BinaryOp::Custom(_) => Err(RenderError::unsupported( - "CustomBinaryOp", - "SQLite does not support custom binary operators.", - )), + BinaryOp::Custom(custom) => { + self.render_operand(left, ctx)?; + custom.render(self, ctx)?; + self.render_operand(right, ctx) + } // power(l, r) — operands rendered once; works in any param mode. BinaryOp::Power => { @@ -781,10 +782,7 @@ impl Renderer for SqliteRenderer { Ok(()) } - Expr::Custom(_) => Err(RenderError::unsupported( - "CustomExpr", - "custom expression must be handled by a wrapping renderer", - )), + Expr::Custom(custom) => custom.render(self, ctx), } } @@ -900,11 +898,8 @@ impl Renderer for SqliteRenderer { self.render_query(query, ctx)?; ctx.paren_close(); } - ConditionNode::Custom(_) => { - return Err(RenderError::unsupported( - "CustomCondition", - "custom condition must be handled by a wrapping renderer", - )); + ConditionNode::Custom(custom) => { + custom.render(self, ctx)?; } } } diff --git a/docs/extensibility.md b/docs/extensibility.md index 95041a6..fb2c47d 100644 --- a/docs/extensibility.md +++ b/docs/extensibility.md @@ -65,51 +65,29 @@ impl CustomExpr for AtTimeZone { fn clone_box(&self) -> Box { Box::new(self.clone()) } -} -``` - -**Step 2: Wrap the renderer and override `render_expr`.** - -```rust -use qcraft_core::render::renderer::Renderer; -use qcraft_core::render::ctx::RenderCtx; -use qcraft_core::ast::expr::Expr; -use qcraft_core::error::RenderResult; -use qcraft_core::delegate_renderer; -use qcraft_postgres::PostgresRenderer; - -pub struct MyRenderer { - inner: PostgresRenderer, -} -impl MyRenderer { - pub fn new() -> Self { - Self { - inner: PostgresRenderer::new(), - } + // The node renders itself. It is handed the renderer, so it recurses back through + // it for any sub-expression it holds — nested expressions, identifier quoting and + // parameter numbering all go through the same dialect and the same RenderCtx. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + // AT TIME ZONE is an operator, so its left-hand side is an operand: + // `a + b AT TIME ZONE 'UTC'` would bind the zone to `b`. render_operand adds + // brackets when the sub-expression's own structure needs them. + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) } -} -impl Renderer for MyRenderer { - fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { - if let Expr::Custom(custom) = expr { - if let Some(atz) = custom.as_any().downcast_ref::() { - self.inner.render_expr(&atz.expr, ctx)?; - ctx.keyword("AT TIME ZONE"); - ctx.string_literal(&atz.zone); - return Ok(()); - } - } - // Fall through to the default renderer for all other expressions - self.inner.render_expr(expr, ctx) + // This node renders an infix operator, so it is not self-delimiting: as the operand + // of `::` it must be bracketed, or `x AT TIME ZONE 'UTC'::date` casts the zone + // literal instead. A node rendering `my_func(x)` would return false here. + fn needs_operand_parens(&self) -> bool { + true } - - // Delegate every other Renderer method to self.inner - delegate_renderer!(self.inner); } ``` -**Step 3: Use it in a query.** +**Step 2: Use it in a query — no renderer wrapping needed.** ```rust let expr = Expr::Custom(Box::new(AtTimeZone { @@ -117,10 +95,34 @@ let expr = Expr::Custom(Box::new(AtTimeZone { zone: "UTC".to_string(), })); -// Use in a SelectColumn, WHERE clause, etc. -let col = SelectColumn::aliased(expr, "created_utc"); +let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(expr, "date"), // brackets are added for you + alias: Some("day".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + ..Default::default() +}; + +let (sql, params) = PostgresRenderer::new().render_query_stmt(&stmt).unwrap(); +// SELECT ("events"."created_at" AT TIME ZONE 'UTC')::date AS "day" FROM "events" ``` +The stock renderer handles it — the knowledge lives in the node, not in a renderer +wrapped around the dialect. The same applies to `CustomCondition`, `CustomBinaryOp` and +the other `Custom*` traits: each has a `render` method with the same contract. qcraft's +own `PgVectorOp` (`<->`, `<#>`, …) is implemented exactly this way, as a `CustomBinaryOp`. + +A `Custom*` node that does not implement `render` is a `RenderError`, never silently +dropped SQL. + +### Wrapping a renderer (`delegate_renderer!`) + +`delegate_renderer!(self.inner)` forwards **every** `Renderer` method to an inner +renderer. Because it emits all of them, it **cannot be combined with overriding one** — +that is a duplicate definition (`error[E0201]`). Use it to wrap a dialect wholesale; to +add a node the renderer does not know, implement `CustomExpr::render` as above instead. + ### The `as_any()` + `downcast_ref` pattern Because `Custom*` traits are trait objects, you need runtime downcasting to access fields on your concrete type. The pattern is: