Skip to content

Commit

Permalink
nom-sql: Rework negative number parsing
Browse files Browse the repository at this point in the history
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 <jason.b@readyset.io>
  • Loading branch information
ethan-readyset committed Feb 22, 2024
1 parent 658018d commit a67c2f6
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 42 deletions.
8 changes: 8 additions & 0 deletions dataflow-expression/tests/mysql_oracle.rs
Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions dataflow-expression/tests/postgres_oracle.rs
Expand Up @@ -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'",
Expand Down
58 changes: 34 additions & 24 deletions nom-sql/src/expression.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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(), "");
Expand Down Expand Up @@ -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();
Expand All @@ -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(), "");
Expand Down
28 changes: 10 additions & 18 deletions nom-sql/src/update.rs
Expand Up @@ -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() {
Expand All @@ -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,
}
Expand Down Expand Up @@ -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() {
Expand All @@ -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,
}
Expand Down

0 comments on commit a67c2f6

Please sign in to comment.