feat(spans): De-duplicate SQL conditions

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+**Features**:
+
+- Group db spans with repeating logical conditions together. ([#2929](https://github.com/getsentry/relay/pull/2929))
+
 **Bug Fixes**:
 
 - Normalize event timestamps before validating them, fixing cases where Relay would drop valid events with reason "invalid_transaction". ([#2878](https://github.com/getsentry/relay/pull/2878))

relay-event-normalization/src/normalize/span/description/sql/mod.rs

@@ -211,8 +211,8 @@ mod tests {
 
     scrub_sql_test!(
         various_parameterized_ins_percentage,
-        "SELECT count() FROM table1 WHERE id IN (%s, %s) AND id IN (%s, %s, %s)",
-        "SELECT count() FROM table1 WHERE id IN (%s) AND id IN (%s)"
+        "SELECT count() FROM table1 WHERE id1 IN (%s, %s) AND id2 IN (%s, %s, %s)",
+        "SELECT count() FROM table1 WHERE id1 IN (%s) AND id2 IN (%s)"
     );
 
     scrub_sql_test!(
@@ -698,6 +698,46 @@ mod tests {
         "UPDATE tbl SET foo = CASE WHEN .. THEN .. END"
     );
 
+    scrub_sql_test!(
+        duplicate_conditions,
+        r#"SELECT *
+        FROM a
+        WHERE a.status = %s
+        AND (
+            (a.id = %s AND a.org = %s)
+            OR (a.id = %s AND a.org = %s)
+            OR (a.id = %s AND a.org = %s)
+            OR (a.id = %s AND a.org = %s)
+        )"#,
+        "SELECT * FROM a WHERE status = %s AND (id = %s AND org = %s)"
+    );
+
+    scrub_sql_test!(
+        duplicate_conditions_or,
+        r#"SELECT *
+        FROM a
+        WHERE a.status = %s
+        OR (
+            (a.id = %s OR a.org = %s)
+            OR (a.id = %s OR a.org = %s)
+            OR (a.id = %s OR a.org = %s)
+            OR (a.id = %s OR a.org = %s)
+        )"#,
+        "SELECT * FROM a WHERE status = %s OR (id = %s OR org = %s)"
+    );
+
+    scrub_sql_test!(
+        non_duplicate_conditions,
+        r#"SELECT *
+        FROM a
+        WHERE a.status = %s
+        AND (
+            (a.id = %s AND a.org2 = %s)
+            OR (a.id = %s AND a.org = %s)
+        )"#,
+        "SELECT * FROM a WHERE status = %s AND ((id = %s AND org2 = %s) OR (id = %s AND org = %s))"
+    );
+
     scrub_sql_test!(
         unique_alias,
         "SELECT pg_advisory_unlock(%s, %s) AS t0123456789abcdef",

relay-event-normalization/src/normalize/span/description/sql/parser.rs

@@ -6,9 +6,9 @@ use itertools::Itertools;
 use once_cell::sync::Lazy;
 use regex::Regex;
 use sqlparser::ast::{
-    AlterTableOperation, Assignment, CloseCursor, ColumnDef, Expr, Ident, ObjectName, Query,
-    Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint, TableFactor,
-    UnaryOperator, Value, VisitMut, VisitorMut,
+    AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, Ident,
+    ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint,
+    TableFactor, UnaryOperator, Value, VisitMut, VisitorMut,
 };
 use sqlparser::dialect::{Dialect, GenericDialect};
 
@@ -268,10 +268,26 @@ impl VisitorMut for NormalizeVisitor {
     fn post_visit_expr(&mut self, expr: &mut Expr) -> ControlFlow<Self::Break> {
         // Casts are omitted for simplification. Because we replace the entire expression,
         // the replacement has to occur *after* visiting its children.
-        if let Expr::Cast { expr: inner, .. } = expr {
-            let mut swapped = Expr::Value(Value::Null);
-            std::mem::swap(&mut swapped, inner);
-            *expr = swapped;
+        match expr {
+            Expr::Cast { expr: inner, .. } => {
+                *expr = take_expr(inner);
+            }
+            Expr::BinaryOp {
+                ref mut left,
+                op: BinaryOperator::Or | BinaryOperator::And,
+                ref right,
+            } => {
+                let is_equal = left == right;
+                if is_equal {
+                    *expr = take_expr(left);
+                }
+            }
+            Expr::Nested(inner) if matches!(inner.as_ref(), &Expr::Nested(_)) => {
+                // Remove multiple levels of parentheses.
+                // These can occur because of the binary op reduction above.
+                *expr = take_expr(inner);
+            }
+            _ => (),
         }
 
         self.current_expr_depth = self.current_expr_depth.saturating_sub(1);
@@ -420,6 +436,15 @@ impl VisitorMut for NormalizeVisitor {
     }
 }
 
+/// Get ownership of an `Expr` and leave a NULL value in its place.
+///
+/// We cannot use [`std::mem::take`], because [`Expr`] does not implement [`Default`].
+fn take_expr(expr: &mut Expr) -> Expr {
+    let mut swapped = Expr::Value(Value::Null);
+    std::mem::swap(&mut swapped, expr);
+    swapped
+}
+
 /// An extension of an SQL dialect that accepts `?`, `%s`, `:c0` as valid input.
 #[derive(Debug)]
 struct DialectWithParameters(Box<dyn Dialect>);