Syntax-based query rewriter #1494

newtoncx · 2019-04-09T03:58:16Z

The code here uses templates across many optimizer files to leverage the old query rewriter and allow it to operate on expression trees. A very simple rewriting task is passing. Use of abstract interfaces may provide a cleaner way to generalize the rewriter, and the code is currently in progress in a separate branch of development.

- pattern - rule - ruleset - group - groupexpression - binding - memo - optimize_context - optimizer_task (TopDownRewrite/BottomUpRewrite) Templates generally followed: template <class Node, class OperatorType, class OperatorExpr> The template instantiation associated with: Node = Operator, OperatorType = OpType, OperatorExpr = OperatorExpression is used primarily by the core Optimizer. All references to the templated files/classes from core optimizer files were instantiated to that. Note worth mentioning: Operator class defines a public interface wrapper around BaseOperatorNode, basically defines a single logical/physical operator. OpType class defines the various logical/physical operations OperatorExpression class is essentially a tree of Operator

Possibly annoying problems w.r.t Peloton/terrier: (1) Use of unique_ptr/raw pointer as opposed to shared_ptr in AbstractExpression (2) AbstractExpression equality comparison method Additional components needed: - Dynamic/template/strategy rule evaluation (particularly comparison) - Repeated/multi-level application of rules - Layer to convert from memo -> AbstractExpression - Some refactoring w.r.t templated code - Better AbsExpr_Container/Expression indirection layer (intended to present a similar interface exposed by Operator/OperatorExpression relied upon by core logic) - Proper memory management strategy (tightly coupled to problem #1)

What still doesn't work/don't care about yet/not done - proper memory management (terrier uses shared_ptr anyways) - other 1-level rewrites, multi-layer rewrites, other expr rewrites - how can we define a grammar to programmatically create these rewrites? (the one we have is way too static...) - in relation to logical equivalence: (1) how do we generate logically equivalent expressions: - multi-pass using generating rules (similar to ApplyRule) OR - from Pattern A, generate logically equivalent set of patterns P OR - transform input expression to match certain specification OR - ??? (2) what operators do we support translating? - probably (a AND b) ====> (b AND a) - probably (a OR b) ====> (b OR a) - probably (a = b) ====> (b = a) - maybe more??? (3) do we want multi level translations? - i.e (a AND b) AND c ====> (a AND (b AND c)) - what order do we do these in? May have to modify these operations: - Some assertions in TopDownRewrite/BottomUpRewrite w.r.t to the iterator - Possibly binding.cpp / optimizer_metadata.h / optimizer_task.cpp Issues still pending: - Comparing Values (Matt email/discussion) - r.rule->Check (terrier issue cmu-db#332)

coveralls · 2019-04-09T05:03:38Z

Coverage decreased (-1.1%) to 75.418% when pulling f4d4e8f on 17zhangw:templatize into 484d76d on cmu-db:master.

lmwnshn · 2019-04-14T20:36:56Z

test/optimizer/rewriter_test.cpp

+TEST_F(RewriterTests, SimpleEqualityTree) {
+  //                      [=]
+  //                  [=]     [=]
+  //                [4] [5] [3] [3]


Maybe add a line to show what you expect it to be rewritten to, i.e. // false.

Also add a comment similar to this one to the other tests so we can easily know what they are testing.

tanujnay112 · 2019-04-14T20:18:33Z

src/include/optimizer/rule_rewrite.h

+  LOW = 1
+};
+
+class ComparatorElimination: public Rule<AbsExpr_Container,ExpressionType,AbsExpr_Expression> {


It would be nice if you added some documentation to this, even if it's simplistic.

tanujnay112 · 2019-04-14T20:19:25Z

src/include/optimizer/memo.h

@@ -68,10 +73,10 @@ class Memo {
  //===--------------------------------------------------------------------===//
  // For rewrite phase: remove and add expression directly for the set
  //===--------------------------------------------------------------------===//
-  void RemoveParExpressionForRewirte(GroupExpression* gexpr) {
+  void RemoveParExpressionForRewirte(GroupExpression<Node,OperatorType,OperatorExpr>* gexpr) {


tanujnay112 · 2019-04-14T20:24:31Z

src/include/optimizer/rewriter.h

+  Rewriter(const Rewriter &) = delete;
+  Rewriter &operator=(const Rewriter &) = delete;
+  Rewriter(Rewriter &&) = delete;
+  Rewriter &operator=(Rewriter &&) = delete;


Not sure if this is necessary as this is default behavior I think

Consider DISALLOW_COPY_AND_MOVE macro.

tanujnay112 · 2019-04-14T20:27:29Z

src/optimizer/memo.cpp

@@ -14,34 +14,71 @@
 #include "optimizer/memo.h"


It would be nice if you added more documentation to memo.h to give a better high-level idea of what these Memo objects are used for.

lmwnshn · 2019-04-14T20:38:15Z

src/include/optimizer/absexpr_expression.h

+// AbsExpr_Container does *not* handle memory correctly w.r.t internal instantiations
+// from Rule transformation. This is since Peloton itself mixes unique_ptrs and
+// hands out raw pointers which makes adding a shared_ptr here extremely problematic.
+// terrier uses only shared_ptr when dealing with AbstractExpression trees.


(All Terrier parser behavior can be changed, just FYI. If anything would make it more convenient for you, make the case for it.)

lmwnshn · 2019-04-14T20:43:29Z

src/include/optimizer/optimizer.h

@@ -85,16 +88,18 @@ class Optimizer : public AbstractOptimizer {

  void Reset() override;

-  OptimizerMetadata &GetMetadata() { return metadata_; }
+  OptimizerMetadata<Operator,OpType,OperatorExpression> &GetMetadata() { return metadata_; }

  /* For test purposes only */


Kind of inherited from bad decisions before, but in terrier we try pretty hard to not have public test-only functions and use FRIEND_TEST instead.

lmwnshn · 2019-04-14T20:45:58Z

src/include/optimizer/rule_rewrite.h

+namespace optimizer {
+
+/* Rules are applied from high to low priority */
+enum class RulePriority : int {


(I know nothing about optimizers) Is priority a well-defined concept for optimization or is this a rough heuristic? What happens if you have two rules of the same priority that can both be applied, is it always arbitrary which should go first?

KatiaVi · 2019-04-15T02:32:15Z

src/include/optimizer/absexpr_expression.h

+    }
+    return false;
+  }
+


This may not be a huge deal since, as you said, the == op doesn't affect correctness but do you intend to implement the == op for rewrites? If so, does there exist an == op for AbstractExpression you can use?

I believe Terrier currently only has the notion of logical equality for abstract expressions.

KatiaVi · 2019-04-15T02:38:57Z

src/include/optimizer/absexpr_expression.h

+      }
+    }
+  }
+


What does Rebuild do in context of its AbsExpr_Container? You could probably add some more documentation for this function.

tanujnay112 · 2019-04-15T08:43:47Z

src/include/optimizer/rule_rewrite.h

+#include "optimizer/rule.h"
+#include "optimizer/absexpr_expression.h"
+
+#include <memory>


nitpick: clang-tidy will complain about the ordering of the imports so be aware of that in the future. I believe native libraries such as should go first here.

tanujnay112 · 2019-04-15T08:44:28Z

src/include/optimizer/absexpr_expression.h

+#include "expression/comparison_expression.h"
+#include "expression/constant_value_expression.h"
+
+#include <memory>


Same nitpick about ordering #include's.

KatiaVi · 2019-04-15T18:13:34Z

src/optimizer/rewriter.cpp

+using GroupExpressionTemplate = GroupExpression<AbsExpr_Container,ExpressionType,AbsExpr_Expression>;
+
+using GroupTemplate = Group<AbsExpr_Container,ExpressionType,AbsExpr_Expression>;
+


Would be nice to see more documentation of the functions in Rewriter as well explanations for the templates you use. Maybe a high level explanation of what the rewriter does at the top of the rewriter.h file would be helpful?

amlatyrngom

This may just be due to Peloton, but the code in some parts is fairly cryptic, so it would help to have more documentation to understand what it's doing. Also, I think terrier may reject PRs that do not contain enough documentation for functions.

amlatyrngom · 2019-04-16T02:02:34Z

src/include/optimizer/optimize_context.h

                  std::shared_ptr<PropertySet> required_prop,
                  double cost_upper_bound = std::numeric_limits<double>::max())
      : metadata(metadata),
        required_prop(required_prop),
        cost_upper_bound(cost_upper_bound) {}

-  OptimizerMetadata *metadata;
+  OptimizerMetadata<Operator,OperatorType,OperatorExpr> *metadata;


Just for style: the rest of the code names class members as metadata_ or required_prop_.
In addition, is there a particular reason why these members are public?

amlatyrngom · 2019-04-16T02:07:09Z

src/include/optimizer/optimizer_task_pool.h

  virtual bool Empty() = 0;
 };

 /**
 * @brief Stack implementation of the task pool
 */
-class OptimizerTaskStack : public OptimizerTaskPool {
+template <class Node, class OperatorType, class OperatorExpr>


Consider using the final keyword if this class is not be inherited.

amlatyrngom · 2019-04-16T02:13:53Z

test/optimizer/rewriter_test.cpp

+TEST_F(RewriterTests, SimpleEqualityTree) {
+  //                      [=]
+  //                  [=]     [=]
+  //                [4] [5] [3] [3]


Also add a comment similar to this one to the other tests so we can easily know what they are testing.

17zhangw added 3 commits April 1, 2019 18:53

newtoncx changed the title ~~Simple rewrite functionality~~ Syntax-based query rewriter Apr 9, 2019

lmwnshn reviewed Apr 14, 2019

View reviewed changes

tanujnay112 reviewed Apr 14, 2019

View reviewed changes

lmwnshn reviewed Apr 14, 2019

View reviewed changes

KatiaVi reviewed Apr 15, 2019

View reviewed changes

lmwnshn requested a review from amlatyrngom April 15, 2019 03:14

lmwnshn assigned newtoncx Apr 15, 2019

tanujnay112 reviewed Apr 15, 2019

View reviewed changes

KatiaVi reviewed Apr 15, 2019

View reviewed changes

amlatyrngom reviewed Apr 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntax-based query rewriter #1494

Syntax-based query rewriter #1494

newtoncx commented Apr 9, 2019

coveralls commented Apr 9, 2019

lmwnshn Apr 14, 2019

amlatyrngom Apr 16, 2019

tanujnay112 Apr 14, 2019

tanujnay112 Apr 14, 2019

tanujnay112 Apr 14, 2019

lmwnshn Apr 14, 2019

tanujnay112 Apr 14, 2019

lmwnshn Apr 14, 2019

lmwnshn Apr 14, 2019

lmwnshn Apr 14, 2019

KatiaVi Apr 15, 2019 •

edited

Loading

lmwnshn Apr 15, 2019

KatiaVi Apr 15, 2019

tanujnay112 Apr 15, 2019

tanujnay112 Apr 15, 2019

KatiaVi Apr 15, 2019

amlatyrngom left a comment

amlatyrngom Apr 16, 2019

amlatyrngom Apr 16, 2019

amlatyrngom Apr 16, 2019

		using GroupExpressionTemplate = GroupExpression<AbsExpr_Container,ExpressionType,AbsExpr_Expression>;

		using GroupTemplate = Group<AbsExpr_Container,ExpressionType,AbsExpr_Expression>;

Syntax-based query rewriter #1494

Are you sure you want to change the base?

Syntax-based query rewriter #1494

Conversation

newtoncx commented Apr 9, 2019

coveralls commented Apr 9, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KatiaVi Apr 15, 2019 • edited 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

amlatyrngom left a 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

KatiaVi Apr 15, 2019 •

edited

Loading