From 0bfdbcaa1ef197cb4b1bb7fa652ba4879b3cb3dc Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 3 Apr 2023 12:33:25 +0900
Subject: [PATCH] revset: don't rewrite '~set & filter' as difference

Since filter is slow in general, its input set should be minimized. This has
measurable impact on artificial query like '~(v0.4.0..) & author(_)'. If it
were evaluated as a difference of sets, all commits would have to be loaded.
---
 lib/src/revset.rs | 72 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index f3de5026f..2cbc258d4 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -1380,6 +1380,8 @@ fn fold_difference(expression: &Rc<RevsetExpression>) -> TransformedExpression {
     transform_expression_bottom_up(expression, |expression| match expression.as_ref() {
         RevsetExpression::Intersection(expression1, expression2) => {
             match (expression1.as_ref(), expression2.as_ref()) {
+                // For '~x & f', don't move filter node 'f' left
+                (_, RevsetExpression::Filter(_) | RevsetExpression::AsFilter(_)) => None,
                 (_, RevsetExpression::NotIn(complement)) => {
                     Some(to_difference(expression1, complement))
                 }
@@ -2490,13 +2492,11 @@ mod tests {
         );
 
         assert_eq!(
-            optimize(parse("present(author(foo) ~ bar)").unwrap()),
-            Rc::new(RevsetExpression::AsFilter(Rc::new(
-                RevsetExpression::Present(
-                    RevsetExpression::filter(RevsetFilterPredicate::Author("foo".to_owned()))
-                        .minus(&RevsetExpression::symbol("bar".to_owned()))
-                )
-            )))
+            optimize(parse("present(foo ~ bar)").unwrap()),
+            Rc::new(RevsetExpression::Present(
+                RevsetExpression::symbol("foo".to_owned())
+                    .minus(&RevsetExpression::symbol("bar".to_owned()))
+            ))
         );
         assert_eq!(
             optimize(parse("present(branches() & all())").unwrap()),
@@ -2766,7 +2766,63 @@ mod tests {
                 ),
             ),
         )
-        "###)
+        "###);
+
+        // '~set & filter()' shouldn't be substituted.
+        insta::assert_debug_snapshot!(
+            optimize(parse("~foo & author(bar)").unwrap()), @r###"
+        Intersection(
+            NotIn(
+                Symbol(
+                    "foo",
+                ),
+            ),
+            Filter(
+                Author(
+                    "bar",
+                ),
+            ),
+        )
+        "###);
+        insta::assert_debug_snapshot!(
+            optimize(parse("~foo & (author(bar) | baz)").unwrap()), @r###"
+        Intersection(
+            NotIn(
+                Symbol(
+                    "foo",
+                ),
+            ),
+            AsFilter(
+                Union(
+                    Filter(
+                        Author(
+                            "bar",
+                        ),
+                    ),
+                    Symbol(
+                        "baz",
+                    ),
+                ),
+            ),
+        )
+        "###);
+
+        // Filter should be moved right of the intersection.
+        insta::assert_debug_snapshot!(
+            optimize(parse("author(foo) ~ bar").unwrap()), @r###"
+        Intersection(
+            NotIn(
+                Symbol(
+                    "bar",
+                ),
+            ),
+            Filter(
+                Author(
+                    "foo",
+                ),
+            ),
+        )
+        "###);
     }
 
     #[test]