readyset-sql-parsing: Wrap expression subparser

mvzink · MohamedAbdeen-rs · commit 236181f7a62e · 2025-04-23T20:55:18.000Z
This allows tests which directly use the expression subparser to parse with both sqlparser-rs and nom-sql to verify parity. Change-Id: I91402127c9e2aa80f0037180ab2514f543214fd2 Reviewed-on: https://gerrit.readyset.name/c/readyset/+/9102 Tested-by: Buildkite CI Reviewed-by: Michael Zink <michael.z@readyset.io> Reviewed-by: Mohamed Yasser Abdelhamed Abdeen <mohamed@readyset.io>
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -223,7 +223,7 @@ slotmap = "1.0.7"
 smallvec = { version = "1.13.2", default-features = false }
 socket2 = "0.5"
 sqlformat = "0.3.1"
-sqlparser = { git = "https://github.com/apache/datafusion-sqlparser-rs", rev = "53aba68e2dc758e591292115b1dd5faf71374346" }
+sqlparser = { git = "https://github.com/apache/datafusion-sqlparser-rs", rev = "ac0367ce1d6f71d80108d9e19be4b6574b832080" }
 strawpoll = "0.2.3"
 streaming-iterator = "0.1"
 stringprep = "0.1.4"
diff --git a/dataflow-expression/Cargo.toml b/dataflow-expression/Cargo.toml
@@ -38,6 +38,7 @@ postgres = { workspace = true, features = [
     "with-serde_json-1",
     "with-bit-vec-0_6",
 ] }
+readyset-sql-parsing = { path = "../readyset-sql-parsing" }
 regex = { workspace = true }
 serial_test = { workspace = true }
 test-utils = { path = "../test-utils" }
diff --git a/dataflow-expression/src/eval.rs b/dataflow-expression/src/eval.rs
@@ -373,10 +373,10 @@ impl Expr {
 #[cfg(test)]
 mod tests {
     use chrono::{NaiveDate, NaiveDateTime, NaiveTime};
-    use nom_sql::parse_expr;
     use readyset_data::{ArrayD, Collation, DfType, Dialect, IxDyn, PgEnumMetadata};
     use readyset_errors::internal;
     use readyset_sql::Dialect::*;
+    use readyset_sql_parsing::parse_expr;
     use serde_json::json;
     use Expr::*;
 
diff --git a/dataflow-expression/src/eval/builtins.rs b/dataflow-expression/src/eval/builtins.rs
@@ -1436,9 +1436,9 @@ mod tests {
     use chrono::NaiveTime;
     use chrono_tz::{Asia, Atlantic};
     use lazy_static::lazy_static;
-    use nom_sql::parse_expr;
     use readyset_errors::{internal, internal_err};
     use readyset_sql::Dialect::*;
+    use readyset_sql_parsing::parse_expr;
     use readyset_util::arbitrary::{
         arbitrary_timestamp_naive_date_time, arbitrary_timestamp_naive_date_time_for_timezone,
     };
@@ -2099,7 +2099,7 @@ mod tests {
 
     #[test]
     fn substring_with_from_and_for() {
-        let expr = parse_and_lower("SUBSTRING('abcdef', c0, c1)", MySQL);
+        let expr = parse_and_lower("substring('abcdef', c0, c1)", MySQL);
         let call_with =
             |from: i64, len: i64| expr.eval::<DfValue>(&[from.into(), len.into()]).unwrap();
 
@@ -2116,21 +2116,21 @@ mod tests {
 
     #[test]
     fn substring_multibyte() {
-        let expr = parse_and_lower("SUBSTRING('é' from 1 for 1)", PostgreSQL);
+        let expr = parse_and_lower("substring('é' from 1 for 1)", PostgreSQL);
         let res = expr.eval::<DfValue>(&[]).unwrap();
         assert_eq!(res, "é".into());
     }
 
     #[test]
     fn substring_with_from() {
-        let expr = parse_and_lower("SUBSTRING('abcdef' from c0)", PostgreSQL);
+        let expr = parse_and_lower("substring('abcdef' from c0)", PostgreSQL);
         let res = expr.eval::<DfValue>(&[2.into()]).unwrap();
         assert_eq!(res, "bcdef".into());
     }
 
     #[test]
     fn substring_with_for() {
-        let expr = parse_and_lower("SUBSTRING('abcdef' for c0)", PostgreSQL);
+        let expr = parse_and_lower("substring('abcdef' for c0)", PostgreSQL);
         let res = expr.eval::<DfValue>(&[3.into()]).unwrap();
         assert_eq!(res, "abc".into());
     }
diff --git a/dataflow-expression/src/lower.rs b/dataflow-expression/src/lower.rs
@@ -1495,10 +1495,10 @@ impl Expr {
 
 #[cfg(test)]
 pub(crate) mod tests {
-    use nom_sql::parse_expr;
     use readyset_data::{Collation, PgEnumMetadata};
     use readyset_sql::ast::{BinaryOperator as AstBinaryOperator, Float, Literal};
     use readyset_sql::Dialect as ParserDialect;
+    use readyset_sql_parsing::parse_expr;
 
     use super::*;
 
@@ -1729,7 +1729,7 @@ pub(crate) mod tests {
 
     #[test]
     fn substr_regular() {
-        let input = parse_expr(ParserDialect::MySQL, "SUBSTR('abcdefghi', 1, 7)").unwrap();
+        let input = parse_expr(ParserDialect::MySQL, "substr('abcdefghi', 1, 7)").unwrap();
         let res = Expr::lower(input, Dialect::DEFAULT_MYSQL, &no_op_lower_context()).unwrap();
         assert_eq!(
             res,
@@ -1755,7 +1755,7 @@ pub(crate) mod tests {
 
     #[test]
     fn substring_regular() {
-        let input = parse_expr(ParserDialect::MySQL, "SUBSTRING('abcdefghi', 1, 7)").unwrap();
+        let input = parse_expr(ParserDialect::MySQL, "substring('abcdefghi', 1, 7)").unwrap();
         let res = Expr::lower(input, Dialect::DEFAULT_MYSQL, &no_op_lower_context()).unwrap();
         assert_eq!(
             res,
@@ -1781,7 +1781,7 @@ pub(crate) mod tests {
 
     #[test]
     fn substring_without_string_arg() {
-        let input = parse_expr(ParserDialect::MySQL, "SUBSTRING(123 from 2)").unwrap();
+        let input = parse_expr(ParserDialect::MySQL, "substring(123 from 2)").unwrap();
         let res = Expr::lower(input, Dialect::DEFAULT_MYSQL, &no_op_lower_context()).unwrap();
         assert_eq!(res.ty(), &DfType::DEFAULT_TEXT);
     }
diff --git a/dataflow-expression/tests/common/mod.rs b/dataflow-expression/tests/common/mod.rs
@@ -1,8 +1,8 @@
 use dataflow_expression::{Expr, LowerContext};
-use nom_sql::parse_expr;
 use readyset_data::{DfType, DfValue};
 use readyset_errors::{internal, ReadySetResult};
 use readyset_sql::ast::{Column, Relation};
+use readyset_sql_parsing::parse_expr;
 
 #[derive(Debug, Clone, Copy)]
 struct TestLowerContext;
diff --git a/dataflow-expression/tests/mysql_oracle.rs b/dataflow-expression/tests/mysql_oracle.rs
@@ -145,10 +145,10 @@ async fn example_exprs_eval_same_as_mysql() {
         "json_overlaps('[]', '[]')",
         "json_overlaps('true', 'true')",
         "json_overlaps('[42]', '[0, 42, 0]')",
-        "SUBSTRING('abcdef', 3)",
-        "SUBSTRING('abcdef', 3, 2)",
-        "SUBSTRING('abcdef', 1, 3)",
-        "SUBSTRING('abcdef', '1', '3')",
+        "substring('abcdef', 3)",
+        "substring('abcdef', 3, 2)",
+        "substring('abcdef', 1, 3)",
+        "substring('abcdef', '1', '3')",
         "concat(1,2)",
         "concat('one',2)",
         "concat('one',2)",
diff --git a/readyset-mir/Cargo.toml b/readyset-mir/Cargo.toml
@@ -25,7 +25,7 @@ readyset-errors = { path = "../readyset-errors" }
 readyset-sql = { path = "../readyset-sql" }
 
 [dev-dependencies]
-nom-sql = { path = "../nom-sql" }
+readyset-sql-parsing = { path = "../readyset-sql-parsing" }
 readyset-tracing = { path = "../readyset-tracing" }
 
 [lints]
diff --git a/readyset-mir/src/rewrite/filters_to_join_keys.rs b/readyset-mir/src/rewrite/filters_to_join_keys.rs
@@ -379,10 +379,10 @@ pub(crate) fn convert_filters_to_join_keys(query: &mut MirQuery<'_>) -> ReadySet
 #[cfg(test)]
 mod tests {
     use common::IndexType;
-    use nom_sql::parse_expr;
     use readyset_client::ViewPlaceholder;
     use readyset_sql::ast::{self, ColumnSpecification, Relation, SqlType};
     use readyset_sql::Dialect;
+    use readyset_sql_parsing::parse_expr;
 
     use super::*;
     use crate::graph::MirGraph;
diff --git a/readyset-sql-parsing/src/lib.rs b/readyset-sql-parsing/src/lib.rs
@@ -1,4 +1,7 @@
-use readyset_sql::{ast::SqlQuery, Dialect};
+use readyset_sql::{
+    ast::{Expr, SqlQuery},
+    Dialect,
+};
 
 #[cfg(feature = "sqlparser")]
 use readyset_sql::{
@@ -471,13 +474,33 @@ fn parse_readyset_query(
 }
 
 #[cfg(feature = "sqlparser")]
-pub fn parse_query(dialect: Dialect, input: impl AsRef<str>) -> Result<SqlQuery, String> {
-    let nom_result = nom_sql::parse_query(dialect, input.as_ref());
+fn parse_readyset_expr(
+    parser: &mut Parser,
+    dialect: Dialect,
+    _input: impl AsRef<str>,
+) -> Result<Expr, ReadysetParsingError> {
+    Ok(parser.parse_expr()?.try_into_dialect(dialect)?)
+}
+
+#[cfg(feature = "sqlparser")]
+fn parse_both_inner<S, T, NP, SP>(
+    dialect: Dialect,
+    input: S,
+    nom_parser: NP,
+    sqlparser_parser: SP,
+) -> Result<T, String>
+where
+    T: PartialEq + std::fmt::Debug,
+    S: AsRef<str>,
+    for<'a> NP: FnOnce(Dialect, &'a str) -> Result<T, String>,
+    for<'a> SP: FnOnce(&mut Parser, Dialect, &'a str) -> Result<T, ReadysetParsingError>,
+{
+    let nom_result = nom_parser(dialect, input.as_ref());
     let sqlparser_dialect = sqlparser_dialect_from_readyset_dialect(dialect);
     let sqlparser_result = Parser::new(sqlparser_dialect.as_ref())
         .try_with_sql(input.as_ref())
         .map_err(Into::into)
-        .and_then(|mut p| parse_readyset_query(&mut p, dialect, input.as_ref()));
+        .and_then(|mut p| sqlparser_parser(&mut p, dialect, input.as_ref()));
 
     match (&nom_result, sqlparser_result) {
         (Ok(nom_ast), Ok(sqlparser_ast)) => {
@@ -515,7 +538,33 @@ pub fn parse_query(dialect: Dialect, input: impl AsRef<str>) -> Result<SqlQuery,
     nom_result
 }
 
+#[cfg(feature = "sqlparser")]
+pub fn parse_query(dialect: Dialect, input: impl AsRef<str>) -> Result<SqlQuery, String> {
+    parse_both_inner(
+        dialect,
+        input,
+        |d, s| nom_sql::parse_query(d, s),
+        |p, d, s| parse_readyset_query(p, d, s),
+    )
+}
+
 #[cfg(not(feature = "sqlparser"))]
 pub fn parse_query(dialect: Dialect, input: impl AsRef<str>) -> Result<SqlQuery, String> {
     nom_sql::parse_query(dialect, input.as_ref())
 }
+
+/// Parses a single expression; only intended for use in tests.
+#[cfg(feature = "sqlparser")]
+pub fn parse_expr(dialect: Dialect, input: impl AsRef<str>) -> Result<Expr, String> {
+    parse_both_inner(
+        dialect,
+        input,
+        |d, s| nom_sql::parse_expr(d, s),
+        |p, d, s| parse_readyset_expr(p, d, s),
+    )
+}
+
+#[cfg(not(feature = "sqlparser"))]
+pub fn parse_expr(dialect: Dialect, input: impl AsRef<str>) -> Result<Expr, String> {
+    nom_sql::parse_expr(dialect, input.as_ref())
+}
diff --git a/readyset-sql-parsing/tests/parity.rs b/readyset-sql-parsing/tests/parity.rs
@@ -328,6 +328,23 @@ fn test_convert_with_using() {
     );
 }
 
+#[test]
+fn test_empty_insert() {
+    // MySQL parses `insert into t () VALUES ...` into `insert into t VALUES ...`
+    check_parse_mysql!("INSERT INTO t VALUES ()");
+}
+
+#[test]
+#[cfg(feature = "sqlparser")]
+#[should_panic(expected = "sqlparser error")]
+fn test_empty_insert_fails_in_postgres() {
+    // Invalid postgres syntax, parsed by nom but not by sqlparser
+    // Invalid because of empty values list
+    check_parse_postgres!("INSERT INTO t VALUES ()");
+    // Invalid because of empty cols list, accepted by mysql thought
+    check_parse_postgres!("INSERT INTO t () VALUES ()");
+}
+
 #[test]
 fn test_column_default_without_parens() {
     check_parse_both!(
@@ -354,10 +371,10 @@ fn test_op_any_all() {
 
 #[test]
 fn test_substring() {
-    check_parse_both!("SELECT SUBSTRING('foo', 1, 2) FROM t");
-    check_parse_both!("SELECT SUBSTR('foo', 1, 2) FROM t");
-    check_parse_both!("SELECT SUBSTRING('foo' FROM 1 FOR 2) FROM t");
-    check_parse_both!("SELECT SUBSTR('foo' FROM 1 FOR 2) FROM t");
+    check_parse_both!("SELECT substring('foo', 1, 2) FROM t");
+    check_parse_both!("SELECT substr('foo', 1, 2) FROM t");
+    check_parse_both!("SELECT substring('foo' FROM 1 FOR 2) FROM t");
+    check_parse_both!("SELECT substr('foo' FROM 1 FOR 2) FROM t");
 }
 
 #[test]
@@ -390,15 +407,6 @@ fn test_unsupported_op() {
     check_parse_postgres!("create cache from selecT * from t where a !~* 'f.*' and a ~ 'g.*'");
 }
 
-#[test]
-#[cfg(feature = "sqlparser")]
-#[should_panic(expected = "sqlparser error")]
-/// Invalid postgres syntax, parsed by nom but not by sqlparser
-fn test_empty_insert_fails_in_postgres() {
-    // Invalid because of empty values list
-    check_parse_postgres!("INSERT INTO t VALUES ()");
-}
-
 #[test]
 #[cfg(feature = "sqlparser")]
 #[should_panic(expected = "sqlparser error")]
diff --git a/readyset-sql-passes/src/expr/constant_fold.rs b/readyset-sql-passes/src/expr/constant_fold.rs
@@ -74,8 +74,8 @@ pub fn constant_fold_expr(expr: &mut Expr, dialect: Dialect) {
 
 #[cfg(test)]
 mod tests {
-    use nom_sql::parse_expr;
     use readyset_sql::DialectDisplay;
+    use readyset_sql_parsing::parse_expr;
 
     use super::*;
 
diff --git a/readyset-sql-passes/src/expr/normalize_negation.rs b/readyset-sql-passes/src/expr/normalize_negation.rs
@@ -101,8 +101,8 @@ pub fn normalize_negation(expr: &mut Expr) {
 
 #[cfg(test)]
 mod tests {
-    use nom_sql::parse_expr;
     use readyset_sql::{Dialect, DialectDisplay};
+    use readyset_sql_parsing::parse_expr;
 
     use super::*;
 
@@ -175,8 +175,9 @@ mod tests {
 
     #[test]
     fn normalize_question_mark() {
-        let mut expr = parse_expr(Dialect::MySQL, "NOT (j ? 'key' AND NOT b)").unwrap();
-        let expected = parse_expr(Dialect::MySQL, "NOT j ? 'key' OR b").unwrap();
+        // The ? Operator is Postgres-specific
+        let mut expr = parse_expr(Dialect::PostgreSQL, "NOT (j ? 'key' AND NOT b)").unwrap();
+        let expected = parse_expr(Dialect::PostgreSQL, "NOT j ? 'key' OR b").unwrap();
         normalize_negation(&mut expr);
         assert_eq!(expr, expected);
     }
diff --git a/readyset-sql/src/ast/create.rs b/readyset-sql/src/ast/create.rs
diff --git a/readyset-sql/src/ast/expression.rs b/readyset-sql/src/ast/expression.rs
diff --git a/readyset-sql/src/ast/sql_type.rs b/readyset-sql/src/ast/sql_type.rs
