From a67c2f6cf3eaa4847a70c78c1765dfc84bf23d2d Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Tue, 20 Feb 2024 12:12:05 -0500 Subject: [PATCH] nom-sql: Rework negative number parsing Previously, we parsed negative numbers to be a UnaryOperator::Neg applied to a Literal. This was incompatible with our autoparameterization AST rewrite pass, which only replaced *Literals* with placeholders. To add support for the autoparameterization of negative literals, this commit updates the way we parse prefix negation in our expression pratt parser such that a negative sign applied to an integer, a float, or a double always produces a literal instead of a UnaryOperator. I considered just updating the autoparameterization rewrite pass to look for and replace `UnaryOperators::Neg`s applied to literals, but this would have meant having to evaluate the negative sign in our rewrite pass, which felt less correct than simplifying our representation of negative numbers in the AST. While doing this work, I discovered that we do not correctly evaluate `-1 = -1` for both MySQL and Postgres. This will be addressed in a follow-up commit. Release-Note-Core: Added support for the autoparamterization of negative numbers Change-Id: I8c11f3ad2d4c35d0f08c9eb4d7abc4684c21f4b1 Reviewed-on: https://gerrit.readyset.name/c/readyset/+/6945 Tested-by: Buildkite CI Reviewed-by: Jason Brown --- dataflow-expression/tests/mysql_oracle.rs | 8 +++ dataflow-expression/tests/postgres_oracle.rs | 6 ++ nom-sql/src/expression.rs | 58 ++++++++++++-------- nom-sql/src/update.rs | 28 ++++------ 4 files changed, 58 insertions(+), 42 deletions(-) diff --git a/dataflow-expression/tests/mysql_oracle.rs b/dataflow-expression/tests/mysql_oracle.rs index 26b3777934..813b902a2f 100644 --- a/dataflow-expression/tests/mysql_oracle.rs +++ b/dataflow-expression/tests/mysql_oracle.rs @@ -93,6 +93,14 @@ async fn example_exprs_eval_same_as_mysql() { for expr in [ "1 != 2", "1 != 1", + // TODO(ethan) These currently fail + // "-1 = -1", + // "-1.0 = -1.0", + // "-1 = -1.0", + "1 = --1", + "1 != -1", + "1.0 = --1.0", + "1.0 != -1.0", "4 + 5", "4 + '5'", "5 > 4", diff --git a/dataflow-expression/tests/postgres_oracle.rs b/dataflow-expression/tests/postgres_oracle.rs index 3fdac9eaa9..9d08ee36ae 100644 --- a/dataflow-expression/tests/postgres_oracle.rs +++ b/dataflow-expression/tests/postgres_oracle.rs @@ -70,6 +70,12 @@ async fn example_exprs_eval_same_as_postgres() { "1 != 1", "4 + 5", "5 > 4", + // TODO(ethan) These currently fail + // "-1 = -1", + // "-1.0 = -1.0", + // "-1 = -1.0", + "1 != -1", + "1.0 != -1.0", "'a' like 'A'", "'a' ilike 'A'", "'a' not like 'a'", diff --git a/nom-sql/src/expression.rs b/nom-sql/src/expression.rs index 91d8449b6c..a1d363cdfc 100644 --- a/nom-sql/src/expression.rs +++ b/nom-sql/src/expression.rs @@ -20,7 +20,7 @@ use serde::{Deserialize, Serialize}; use test_strategy::Arbitrary; use crate::common::{column_identifier_no_alias, function_expr, ws_sep_comma}; -use crate::literal::literal; +use crate::literal::{literal, Double, Float}; use crate::select::nested_selection; use crate::set::{variable_scope_prefix, Variable}; use crate::sql_type::{mysql_int_cast_targets, type_identifier}; @@ -1216,10 +1216,35 @@ where _ => unreachable!("Invalid fixity for prefix op"), }; - Ok(Expr::UnaryOp { - op, - rhs: Box::new(rhs), - }) + if op == UnaryOperator::Neg { + match rhs { + Expr::Literal(Literal::UnsignedInteger(i)) => { + Ok(Expr::Literal(Literal::Integer(-(i as i64)))) + } + Expr::Literal(Literal::Integer(i)) => Ok(Expr::Literal(Literal::Integer(-i))), + Expr::Literal(Literal::Float(Float { value, precision })) => { + Ok(Expr::Literal(Literal::Float(Float { + value: -value, + precision, + }))) + } + Expr::Literal(Literal::Double(Double { value, precision })) => { + Ok(Expr::Literal(Literal::Double(Double { + value: -value, + precision, + }))) + } + rhs => Ok(Expr::UnaryOp { + op, + rhs: Box::new(rhs), + }), + } + } else { + Ok(Expr::UnaryOp { + op, + rhs: Box::new(rhs), + }) + } } fn postfix( @@ -1699,10 +1724,7 @@ mod tests { assert_eq!( res.unwrap().1, Expr::Cast { - expr: Box::new(Expr::UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Integer(128))), - }), + expr: Box::new(Expr::Literal(Literal::Integer(-128))), ty: SqlType::UnsignedBigInt(None), postgres_style: false } @@ -2267,10 +2289,7 @@ mod tests { #[test] fn neg_integer() { let qs = b"-256"; - let expected = Expr::UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Integer(256))), - }; + let expected = Expr::Literal(Literal::Integer(-256)); let (remaining, result) = to_nom_result(expression(Dialect::MySQL)(LocatedSpan::new(qs))).unwrap(); assert_eq!(std::str::from_utf8(remaining).unwrap(), ""); @@ -2315,10 +2334,7 @@ mod tests { let qs = b"NOT -1"; let expected = Expr::UnaryOp { op: UnaryOperator::Not, - rhs: Box::new(Expr::UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Integer(1))), - }), + rhs: Box::new(Expr::Literal(Literal::Integer(-1))), }; let (remaining, result) = to_nom_result(expression(Dialect::MySQL)(LocatedSpan::new(qs))).unwrap(); @@ -2329,13 +2345,7 @@ mod tests { #[test] fn neg_neg() { let qs = b"--1"; - let expected = Expr::UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Integer(1))), - }), - }; + let expected = Expr::Literal(Literal::Integer(1)); let (remaining, result) = to_nom_result(expression(Dialect::MySQL)(LocatedSpan::new(qs))).unwrap(); assert_eq!(std::str::from_utf8(remaining).unwrap(), ""); diff --git a/nom-sql/src/update.rs b/nom-sql/src/update.rs index 3afbe9a5a8..5ba4e1fe47 100644 --- a/nom-sql/src/update.rs +++ b/nom-sql/src/update.rs @@ -162,8 +162,7 @@ mod tests { use super::*; use crate::column::Column; use crate::table::Relation; - use crate::Expr::UnaryOp; - use crate::{BinaryOperator, Double, FunctionExpr, ItemPlaceholder, UnaryOperator}; + use crate::{BinaryOperator, Double, FunctionExpr, ItemPlaceholder}; #[test] fn updated_with_neg_float() { @@ -185,13 +184,10 @@ mod tests { table: Relation::from("stories"), fields: vec![( Column::from("hotness"), - UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Double(Double { - value: 19216.5479744, - precision: 7, - }))) - }, + Expr::Literal(Literal::Double(Double { + value: -19216.5479744, + precision: 7, + })) )], where_clause: expected_where_cond, } @@ -261,8 +257,7 @@ mod tests { use super::*; use crate::column::Column; use crate::table::Relation; - use crate::Expr::UnaryOp; - use crate::{BinaryOperator, Double, UnaryOperator}; + use crate::{BinaryOperator, Double}; #[test] fn updated_with_neg_float() { @@ -284,13 +279,10 @@ mod tests { table: Relation::from("stories"), fields: vec![( Column::from("hotness"), - UnaryOp { - op: UnaryOperator::Neg, - rhs: Box::new(Expr::Literal(Literal::Double(Double { - value: 19216.5479744, - precision: 7, - }))) - }, + Expr::Literal(Literal::Double(Double { + value: -19216.5479744, + precision: 7, + })) ),], where_clause: expected_where_cond, }