Normalize OR expressions referencing the same symbol as IN #2698

Praveen2112 · 2020-01-31T18:05:46Z

Supersedes #2374

kokosing · 2020-02-03T10:10:59Z

presto-main/src/main/java/io/prestosql/sql/planner/OrExpressionNormalizer.java

+    private OrExpressionNormalizer()
+    {}
+
+    public static Expression rewrite(Metadata metadata, Expression expression)


s/rewrite/normalizeOrExpressions

kokosing · 2020-02-03T10:13:55Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/NormalizeOrExpressions.java

+
+import static io.prestosql.sql.planner.OrExpressionNormalizer.rewrite;
+
+public class NormalizeOrExpressions


Instead of creating this class maybe you could introduce a factory method ExpressionRewriteRuleSet.of(OrExpressionNormalizer::rewrite)?

kokosing · 2020-02-03T10:15:50Z

presto-main/src/test/java/io/prestosql/sql/planner/TestOrExpressionNormalizer.java

+    @Test
+    public void testSimpleOrExpressions()
+    {
+        assertExpression("x = 0 or x = 1 or x = 2 or x = 3", "x in (0, 1, 2, 3)");


I would format this as:

assertExpression( "x = 0 or x = 1 or x = 2 or x = 3", "x in (0, 1, 2, 3)");

it is much easier to compare expressions that way

kokosing · 2020-02-03T10:17:16Z

presto-main/src/test/java/io/prestosql/sql/planner/TestOrExpressionNormalizer.java

+    public void testSimpleOrExpressions()
+    {
+        assertExpression("x = 0 or x = 1 or x = 2 or x = 3", "x in (0, 1, 2, 3)");
+        assertExpression("(x = 0 or x = 1) or (x = 2 or x = 3)", "x in (0, 1, 2, 3)");


capitalize OR and AND

kokosing · 2020-02-03T10:17:45Z

presto-main/src/test/java/io/prestosql/sql/planner/TestOrExpressionNormalizer.java

+        assertExpression("a = 0 or a = 1 or x = 2 or x = 3", "a in (0, 1) or x in (2, 3)");
+        assertExpression("(a = 0 or x = 1) or (a = 2 or x = 3)", "a in (0, 2) or x in (1, 3)");
+        assertExpression("a = 0 or (a = 1 or (x = 2 or x = 3))", "a in (0, 1) or x in (2, 3)");
+        assertExpression("(a = 0 or a = 1) and (x = 2 or x = 3)", "a in (0, 1) and x in (2, 3)");


can you please add tests with AND and OR mixed in single expression?

kokosing · 2020-02-03T10:19:45Z

presto-main/src/test/java/io/prestosql/sql/planner/TestOrExpressionNormalizer.java

+        assertExpression("(a = 0 or x = 1) or (a = 2 or x = 3)", "a in (0, 2) or x in (1, 3)");
+        assertExpression("a = 0 or (a = 1 or (x = 2 or x = 3))", "a in (0, 1) or x in (2, 3)");
+        assertExpression("(a = 0 or a = 1) and (x = 2 or x = 3)", "a in (0, 1) and x in (2, 3)");
+    }


can you please add a test with x = a or x = b or x IN (2,3,4) or x IN (SELECT 1)

Also can you please add such tests with actual query using tpch connector to make sure that results are correct.

kasiafi · 2020-02-03T13:53:16Z

Some general comments:

the overriden visitor methods aren’t recursive. If you hit a ComparisonExpression or InPredicate or AND-expression, you don’t explore the subexpressions.
This could be fixed by returning treeRewriter.defaultRewrite() or returning null, which would also trigger default rewrite instead of returning the original expression.
please add tests for nested rewrite.

kasiafi · 2020-02-03T13:53:39Z

How do you take care of a case when the normalized expression is equal to the original expression? I’m not sure, but if you return the rewritten equivalent instead of the original instance, it may cause optimizer loop.
Please add a query test with something like x IN (1, 2) OR y IN (3, 4).

kasiafi · 2020-02-03T13:54:30Z

This change could be extended to optimize a tree of nested OR-expressions. To achieve that, you should collect the context over the whole OR-tree and finally rewrite. You could check the class EliminateCrossJoins for inspiration.

kokosing · 2020-02-04T08:24:23Z

presto-main/src/main/java/io/prestosql/sql/planner/OrExpressionNormalizer.java

+
+        public Visitor(Metadata metadata)
+        {
+            this.metadata = metadata;


requireNonNull

kokosing · 2020-02-04T08:24:52Z

presto-main/src/main/java/io/prestosql/sql/planner/OrExpressionNormalizer.java

+        }
+
+        @Override
+        public Expression rewriteExpression(Expression expression, Optional<Context> context, ExpressionTreeRewriter<Optional<Context>> treeRewriter)


move this method to the bottom (or to the top), after (or before) any other more specialized method

kokosing · 2020-02-04T08:26:52Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/NormalizeOrExpressions.java

+
+import static io.prestosql.sql.planner.OrExpressionNormalizer.rewrite;
+
+public class NormalizeOrExpressions


kokosing · 2020-02-04T08:37:13Z

presto-main/src/main/java/io/prestosql/sql/planner/OrExpressionNormalizer.java

+            ComparisonExpression rewrittenExpression = treeRewriter.defaultRewrite(comparisonExpression, Optional.empty());
+            if (rewrittenExpression.getOperator().equals(EQUAL) && context.isPresent()) {
+                context.get().addMapping(comparisonExpression.getLeft(), comparisonExpression.getRight());
+                return FALSE_LITERAL;


you don't need this return FALSE_LITERAL (and any other)

kokosing · 2020-02-04T08:39:52Z

presto-main/src/main/java/io/prestosql/sql/planner/OrExpressionNormalizer.java

+
+    public static Expression normalizeOrExpressions(Metadata metadata, Expression expression)
+    {
+        return ExpressionTreeRewriter.rewriteWith(new Visitor(metadata), expression, Optional.empty());


To me it is a bit misleading that rewrite sometimes does not rewrite but only collect information in context. Maybe you could do it in two phases. For every logical binary expression you could collect with simple visitor (not rewriter) expressions. So rewriting visitor would implement only rewriteLogicalBinaryExpression.

#2698 (comment)

kokosing · 2020-02-04T08:42:06Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/NormalizeOrExpressions.java

+{
+    public NormalizeOrExpressions(Metadata metadata)
+    {
+        super(((expression, context) -> normalizeOrExpressions(metadata, expression)));


I think this rewrite should be part of SimplifyExpressions

sopel39 · 2020-10-31T20:46:59Z

@Praveen2112 what's status of this?

Praveen2112 · 2020-11-01T09:05:29Z

I'm working on a revamp for the same PR

cla-bot bot added the cla-signed label Jan 31, 2020

Praveen2112 force-pushed the or_expression_normalizer branch 7 times, most recently from 8e3f40f to 53e09e8 Compare February 2, 2020 03:05

kokosing reviewed Feb 3, 2020

View reviewed changes

Normalize OR expressions referencing the same symbol as IN

a9cc6a5

Praveen2112 force-pushed the or_expression_normalizer branch from 53e09e8 to a9cc6a5 Compare February 4, 2020 06:34

kokosing reviewed Feb 4, 2020

View reviewed changes

weiatwork mentioned this pull request Jul 2, 2021

Relax the restriction on predicate pushdown over projection #8451

Closed

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:14

Praveen2112 closed this Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize OR expressions referencing the same symbol as IN #2698

Normalize OR expressions referencing the same symbol as IN #2698

Praveen2112 commented Jan 31, 2020 •

edited by sopel39

Loading

kokosing Feb 3, 2020

kokosing Feb 3, 2020

kokosing Feb 4, 2020

kokosing Feb 3, 2020

kokosing Feb 3, 2020

kokosing Feb 3, 2020

kokosing Feb 3, 2020

kasiafi commented Feb 3, 2020

kasiafi commented Feb 3, 2020

kasiafi commented Feb 3, 2020

kokosing Feb 4, 2020

kokosing Feb 4, 2020

kokosing Feb 4, 2020

kokosing Feb 4, 2020

kokosing Feb 4, 2020

findepi Feb 4, 2020

kokosing Feb 4, 2020

sopel39 commented Oct 31, 2020

Praveen2112 commented Nov 1, 2020


		import static io.prestosql.sql.planner.OrExpressionNormalizer.rewrite;

		public class NormalizeOrExpressions

Normalize OR expressions referencing the same symbol as IN #2698

Normalize OR expressions referencing the same symbol as IN #2698

Conversation

Praveen2112 commented Jan 31, 2020 • edited by sopel39 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiafi commented Feb 3, 2020

kasiafi commented Feb 3, 2020

kasiafi commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 commented Oct 31, 2020

Praveen2112 commented Nov 1, 2020

Praveen2112 commented Jan 31, 2020 •

edited by sopel39

Loading