Syntax-Based Query Rewriter, Code Review #2 #1495

esargent28 · 2019-05-05T03:58:07Z

New additions:

More rules (transitivity of tuple-tuple and tuple-constant expressions such as "A=B and B=3", boolean short-circuiting)
Basic catalog-based rules
More documentation

Our full design doc and review guide can be found here.

- 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)

AbstractNode will provide interface for Operator and eventually AbstractExpressions as well. Note there are a few road blocks before the rest of the rewriter can be changed to cleanly use abstract classes: (1) Similarly abstract OperatorExpressions. (2) We will have to find a good place to hide OpType, which is currently an enum type (cannot be abstracted) and pervades the code base. This may be solved by abstracting at the group level, but will have to look into it. (3) Need to clean up and separate interfaces between AbstractNode, OperatorNode, and Operator classes.

Abstract nodes were implemented in 209c46a. This is essentially just refactoring and plugging in abstract nodes throughout the optimizer. The abstract interface exposes OpType and ExpressionType for now, which ideally will be fixed later. Work remaining for abstracting OperatorExpression.

Still need to make fixes to Pattern to support both OpType and ExpType without templatizing. Will also need to clean up code after before build will work.

lmwnshn · 2019-05-12T00:18:22Z

src/binder/bind_node_visitor.cpp

@@ -250,6 +252,21 @@ void BindNodeVisitor::Visit(expression::TupleValueExpression *expr) {
    expr->SetColName(col_name);
    expr->SetValueType(value_type);
    expr->SetBoundOid(col_pos_tuple);
+
+    // TODO(esargent): Uncommenting the following code makes AddressSanitizer get mad at me with a


This seems like a good issue to file, so that we don't make the same mistake when we write a new binder.

lmwnshn · 2019-05-12T00:21:57Z

src/include/optimizer/absexpr_expression.h

+//
+// absexpr_expression.h
+//
+// Identification: src/include/optimizer/absexpr_expression.h


In general, if I understand correctly, it is more convenient for you to be operating on something that is not directly the parser's expressions. We are bringing in logical operators or something like that (PR 232) in terrier, which are the outputs of the binder and inputs to the optimizer. This seems like it might belong there, then. I see that you note this in your design doc.

lmwnshn · 2019-05-12T01:41:47Z

src/include/optimizer/rule.h

 class Rule {
 public:
  virtual ~Rule(){};

+  // TODO(ncx): pattern


Could you elaborate on this TODO?

lmwnshn · 2019-05-12T01:51:48Z

src/include/optimizer/rule_rewrite.h

+  MEDIUM = 2,
+  LOW = 1
+};
+


This seems like a good place to use macros. All the header stubs look the same.

lmwnshn · 2019-05-12T02:11:24Z

src/optimizer/group.cpp

+
+  // Additional assertion checks for AddExpression() with AST rewriting
+  // TODO(ncx): get group expression type
+  // if (std::is_same<Node, AbsExpr_Container>::value) {


Is there a reason these are currently commented out?

amlatyrngom

Adding comments in .h files (like rule_rewrite.h) could be useful to someone just reviewing the code without going deep into the implementation.

amlatyrngom · 2019-05-12T01:53:43Z

src/include/optimizer/absexpr_expression.h

+//
+// This is done to export the correct interface from the wrapped AbstractExpression
+// to the rest of the core rule/optimizer code/logic.
+class AbsExpr_Container: public AbstractNode {


Follow naming convention: AbsExprContainer.

amlatyrngom · 2019-05-12T01:54:53Z

src/include/optimizer/absexpr_expression.h

+      return expr->GetExpressionName();
+    }
+
+    return "Undefined";


Is this case supposed to happen? If not, throw an exception instead?

amlatyrngom · 2019-05-12T01:56:22Z

src/include/optimizer/absexpr_expression.h

+};
+
+
+class AbsExpr_Expression: public AbstractNodeExpression {


Same comment about the naming convention.

src/include/optimizer/abstract_node_expression.h

tanujnay112 · 2019-05-12T09:01:12Z

src/include/optimizer/rewriter.h

+  void RewriteLoop(int root_group_id);
+
+  std::shared_ptr<AbsExpr_Expression> ConvertToAbsExpr(const expression::AbstractExpression *expr);
+  std::shared_ptr<GroupExpression> RecordTreeGroups(const expression::AbstractExpression *expr);


Missing documentation on these

tanujnay112 · 2019-05-12T09:01:35Z

src/include/optimizer/rewriter.h

+
+  DISALLOW_COPY_AND_MOVE(Rewriter);
+
+  OptimizerMetadata &GetMetadata() { return metadata_; }


Doxygen will complain about a lack of doxygen documentation on public functions.

tanujnay112 · 2019-05-12T09:05:35Z

src/include/optimizer/abstract_node.h

+
+  ~AbstractNode() {}
+
+  virtual void Accept(OperatorVisitor *v) const = 0;


Documentation on this would be really useful

tanujnay112 · 2019-05-12T09:08:45Z

test/optimizer/rewriter_test.cpp

+  Rewriter *rewriter = new Rewriter();
+  auto rewrote = rewriter->RewriteExpression(common);
+
+  delete rewriter;


The convention that I think we usually use is to use smart pointers with a custom scope to achieve the same thing.

tanujnay112 · 2019-05-12T09:49:14Z

test/optimizer/rewriter_test.cpp

+TEST_F(RewriterTests, SingleCompareEqualRewritePassTrue) {
+  type::Value leftValue = type::ValueFactory::GetIntegerValue(4);
+  type::Value rightValue = type::ValueFactory::GetIntegerValue(4);
+  auto left = new expression::ConstantValueExpression(leftValue);


Write in comments what this one is doing even though it may be relatively obvious.

tanujnay112 · 2019-05-12T09:51:24Z

src/optimizer/rule_rewrite.cpp

+                                      std::vector<std::shared_ptr<AbstractNodeExpression>> &transformed,
+                                      UNUSED_ATTRIBUTE OptimizeContext *context) const {
+  (void)transformed;
+  (void)context;


You already marked context as UNUSED_ATTRIBUTE on line 49. Take that macro out then.

tanujnay112 · 2019-05-12T09:55:11Z

src/include/optimizer/optimizer_metadata.h

@@ -19,6 +19,8 @@
 #include "optimizer/rule.h"
 #include "settings/settings_manager.h"

+#include <memory>
+


Some documentation of what OptimizerMetadata contains in optimizer_metadata would be nice as well.

KatiaVi · 2019-05-12T12:47:42Z

src/include/expression/abstract_expression.h

@@ -111,6 +111,24 @@ class AbstractExpression : public Printable {
    children_[index].reset(expr);
  }

+  void ClearChildren() {


Great documentation for this function and how it fits in with the query rewriting process as a whole. Is this function only used once during the rewriting phase for a single query? If not, have you guys looked at how the rewriter’s performance is affected?

This function is only used once during the rewriting phase in Peloton. As mentioned I believe in rewriter.cpp, this function is solely intended for ensuring that our rewriter manages state (i.e memory, pointers) correctly.

KatiaVi · 2019-05-12T13:01:28Z

src/include/optimizer/absexpr_expression.h

+    return false;
+  }
+
+  bool operator==(const AbsExpr_Container &r) {


So does this == basically check if at least one of the AbsExpr_Container nodes is not a nullptr? Would it make sense to have this function call the more extensive checks on its contents? Or would this significantly affect your performance.

We believe extensive checks on the contents are important for performance reasons but not necessary for correctness. Due to Peloton's design, we could not use operator== directly (i.e ConstantValueExpression does not check the Value). We believe "terrier" implements operator== correctly.

KatiaVi · 2019-05-12T14:06:38Z

src/include/expression/group_marker_expression.h

+// GroupMarkerExpression
+//===----------------------------------------------------------------------===//
+
+class GroupMarkerExpression : public AbstractExpression {


Could you add some brief high-level documentation about GroupMarkerExpression and how it is used?

17zhangw and others added 9 commits April 1, 2019 18:53

Adding AbstractNodeExpression interface.

3266f29

Merge branch 'templatize' into abstract.

721323c

Still need to make fixes to Pattern to support both OpType and ExpType without templatizing. Will also need to clean up code after before build will work.

Ported equivalent transforms and other rules onto AbstractNodes

5caae6d

Ported not-null foreign keys and short-circuiting

e3ac1ba

17zhangw force-pushed the equiv-experimental branch from c55babe to e3ac1ba Compare May 5, 2019 07:47

lmwnshn reviewed May 12, 2019

View reviewed changes

amlatyrngom reviewed May 12, 2019

View reviewed changes

tanujnay112 reviewed May 12, 2019

View reviewed changes

KatiaVi reviewed May 12, 2019

View reviewed changes

Erik Sargent and others added 7 commits May 14, 2019 18:49

Added documentation on rule_rewrite.h

4984dbf

Switched order of include expressions in abstract_node_expression.h

787cdfd

Swapped order of #includes in abstract_node_expression.h

23a7fbf

GroupMarkerExpression, Rewriter, AbstractNode docs

da839e8

Naming conventions and style fixes.

a0315c3

Addressing TODOs before code merge.

010407b

Merge branch 'erik-docs' into equiv-experimental

1d9bdd1

newtoncx mentioned this pull request May 15, 2019

Syntax-Based Query Rewriter, Code Drop #1496

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntax-Based Query Rewriter, Code Review #2 #1495

Syntax-Based Query Rewriter, Code Review #2 #1495

esargent28 commented May 5, 2019

lmwnshn May 12, 2019

lmwnshn May 12, 2019 •

edited

Loading

lmwnshn May 12, 2019

newtoncx May 14, 2019

lmwnshn May 12, 2019

newtoncx May 14, 2019

lmwnshn May 12, 2019

newtoncx May 14, 2019

amlatyrngom left a comment

amlatyrngom May 12, 2019

newtoncx May 14, 2019

amlatyrngom May 12, 2019

newtoncx May 14, 2019

amlatyrngom May 12, 2019

newtoncx May 14, 2019

tanujnay112 May 12, 2019

17zhangw May 14, 2019

tanujnay112 May 12, 2019

17zhangw May 14, 2019

tanujnay112 May 12, 2019

17zhangw May 14, 2019

tanujnay112 May 12, 2019

tanujnay112 May 12, 2019

17zhangw May 14, 2019

tanujnay112 May 12, 2019

17zhangw May 14, 2019

tanujnay112 May 12, 2019

KatiaVi May 12, 2019

17zhangw May 14, 2019

KatiaVi May 12, 2019

17zhangw May 14, 2019

KatiaVi May 12, 2019

17zhangw May 14, 2019


		DISALLOW_COPY_AND_MOVE(Rewriter);

		OptimizerMetadata &GetMetadata() { return metadata_; }


		~AbstractNode() {}

		virtual void Accept(OperatorVisitor *v) const = 0;

Syntax-Based Query Rewriter, Code Review #2 #1495

Are you sure you want to change the base?

Syntax-Based Query Rewriter, Code Review #2 #1495

Conversation

esargent28 commented May 5, 2019

Choose a reason for hiding this comment

lmwnshn May 12, 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

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

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

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

Choose a reason for hiding this comment

lmwnshn May 12, 2019 •

edited

Loading