[15721] Support for Outer Join #1280

zhaoguoquan94 · 2018-04-11T19:42:46Z

This PR adds support for [Left | Right | Outer] JOIN queries in peloton's query optimizer.

Major changes:

Replace InnerJoin* Rules with Join* Rules which support not only inner join transformation rules but also outer join rules. [rule_impls.h, rule.cpp]
Add new Join operators [query_to_operator_transformer.cpp, operators.h,operator_node.h, internal_types.h]
New physical plan generators[plan_generator.cpp].
Bug fixes.
Test cases.

Current Status

two test cases still fails because old bug in optimizer. [planner_equality_test, optimizer_sql_test]
Unknown bug in executor which generate incorrect results given correct plan.

coveralls · 2018-04-11T21:19:25Z

Coverage decreased (-0.07%) to 77.381% when pulling bb50222 on zhaoguoquan94:optimizer_project into f6446ba on cmu-db:master.

nappelson

did a first passthrough. will be doing another tomorrow

nappelson · 2018-04-17T03:40:56Z

script/testing/junit/OptimizerTest.java

+                Statement stmt = conn.createStatement();
+                ResultSet resultSet = stmt.executeQuery("SELECT * FROM t1 LEFT JOIN t2 ON t1.a=t2.d;");) {
+            assertTrue(resultSet.next());
+            assertEquals(3, resultSet.getInt(4));


This along with other assertEquals could probably be collapsed into a helper that takes in a map of expected values to the input into getInt to avoid writing assertEquals many times

nappelson · 2018-04-17T03:43:13Z

script/testing/junit/OptimizerTest.java

+            }
+            ResultSet resultSet = stmt.executeQuery("SELECT COUNT(*) FROM t1;");
+            resultSet.next();
+            assertEquals(3, resultSet.getInt(1));


probably shouldn't be doing asserts in the Setup phase for tests. make any assertions about the data in a separately contained test. I think the purpose of Setup and Teardown methods should only be for creating and deleting your testing environment per test.

nappelson · 2018-04-17T03:48:44Z

src/include/optimizer/operators.h

+//===--------------------------------------------------------------------===//
+class PhysicalNLJoin : public OperatorNode<PhysicalNLJoin> {
+ public:
+  static Operator make(


add documentation about any public methods

nappelson · 2018-04-17T03:50:32Z

src/include/optimizer/operators.h

+class PhysicalNLJoin : public OperatorNode<PhysicalNLJoin> {
+ public:
+  static Operator make(
+      JoinType _type, std::vector<AnnotatedExpression> conditions,


it looks like you are using a leading underscore to denote _type from the member variable type. you should do the same for left_keys and right_keys if you are using that style

I don't think we want to use leading underscores for this. AFAIK, we don't use that anywhere else in the code.

nappelson · 2018-04-17T03:51:30Z

src/include/planner/abstract_join_plan.h

@@ -87,6 +87,10 @@ class AbstractJoinPlan : public AbstractPlan {
  virtual void HandleSubplanBinding(bool from_left,
                                    const BindingContext &input) = 0;

+  const std::string GetPredicateInfo() const {


add documentation for public method

nappelson · 2018-04-17T03:55:28Z

src/include/optimizer/operators.h

+class PhysicalNLJoin : public OperatorNode<PhysicalNLJoin> {
+ public:
+  static Operator make(
+      JoinType _type, std::vector<AnnotatedExpression> conditions,


probably should pass the conditions by ref

nappelson · 2018-04-17T04:04:16Z

src/optimizer/operators.cpp

+
+hash_t PhysicalNLJoin::Hash() const {
+  hash_t hash = BaseOperatorNode::Hash();
+  for (auto &expr : left_keys)


can these be for (cost auto &...)? Generally good to prefer using const where possible. Based on an initial glance looks like it could be but i may have missed something

nappelson · 2018-04-17T04:04:43Z

src/optimizer/operators.cpp

+
+hash_t PhysicalHashJoin::Hash() const {
+  hash_t hash = BaseOperatorNode::Hash();
+  for (auto &expr : left_keys)


same as above regarding constness

GustavoAngulo

Looks good. Consolidating joins like how you did was a good decision.

GustavoAngulo · 2018-04-16T15:15:39Z

src/optimizer/cost_calculator.cpp

+      memo_->GetGroupByID(gexpr_->GetChildGroupId(1))->GetNumRows();
+
+  output_cost_ = left_child_rows * right_child_rows * DEFAULT_TUPLE_COST;
+}
 void CostCalculator::Visit(UNUSED_ATTRIBUTE const PhysicalInnerNLJoin *op) {


Are the cost functions below (PhysicalInnerNLJoin, PhysicalLeftNLJoin, etc) still needed with the addition of the two new cost functions (PhysicalNLJoin, PhysicalHashJoin)

GustavoAngulo · 2018-04-16T15:19:28Z

src/optimizer/cost_calculator.cpp

+  auto right_child_rows =
+      memo_->GetGroupByID(gexpr_->GetChildGroupId(1))->GetNumRows();
+
+  output_cost_ = left_child_rows * right_child_rows * DEFAULT_TUPLE_COST;


This cost is different from the original cost here. Is this change intended?

GustavoAngulo · 2018-04-17T22:15:03Z

src/include/optimizer/rule_impls.h


  bool Check(std::shared_ptr<OperatorExpression> plan,
             OptimizeContext *context) const override;

  void Transform(std::shared_ptr<OperatorExpression> input,
                 std::vector<std::shared_ptr<OperatorExpression>> &transformed,
                 OptimizeContext *context) const override;
+ private:
+  bool StrongPredicate(std::shared_ptr<OperatorExpression> plan,


There should be some documentation as to why a strong predicate check is needed for outer join associativity.

@GustavoAngulo Where do you want them to add that?

We don't really have a central documentation for the optimizer yet, so above the implementation of the StrongPredicate function should be good.

Hey, can we add this in https://github.com/cmu-db/peloton/blob/master/src/include/optimizer/util.h? It makes more sense add it as a helper function instead of a member function of the rule.

GustavoAngulo · 2018-04-17T22:21:55Z

src/optimizer/plan_generator.cpp

+  join_plan->AddChild(move(hash_plan));
+  output_plan_ = move(join_plan);
+}
+
 void PlanGenerator::Visit(const PhysicalInnerNLJoin *op) {


Delete the methods for PhysicalInnerNLJoin and PhysicalInnerHashJoin if we are consolidating Inner+Outer joins

GustavoAngulo · 2018-04-17T22:26:10Z

src/optimizer/rule_impls.cpp

  (void)context;
  (void)plan;
  return true;
 }

-void InnerJoinToInnerNLJoin::Transform(
+void JoinToNLJoin::Transform(
    std::shared_ptr<OperatorExpression> input,
    std::vector<std::shared_ptr<OperatorExpression>> &transformed,
    UNUSED_ATTRIBUTE OptimizeContext *context) const {
  // first build an expression representing hash join


Comment should be "representing nested loop join"

dasteere · 2018-04-18T22:34:28Z

script/testing/junit/OptimizerTest.java

+            e.printStackTrace();
+            fail();
+        }
+        try (


I would suggest splitting these into different tests. That way if it fails it will be quicker to identify where it went wrong.

dasteere · 2018-04-18T22:34:39Z

script/testing/junit/OptimizerTest.java

+            e.printStackTrace();
+            fail();
+        }
+        try (


nappelson · 2018-04-18T23:47:09Z

src/optimizer/operators.cpp

+  hash_t hash = BaseOperatorNode::Hash();
+  for (auto &expr : left_keys)
+    hash = HashUtil::CombineHashes(hash, expr->Hash());
+  for (auto &expr : right_keys)


same as above

nappelson · 2018-04-19T00:02:59Z

src/optimizer/operators.cpp

+  return hash;
+}
+
+bool PhysicalHashJoin::operator==(const BaseOperatorNode &r) {


it seems that this method shares a lot of code with the PhysicalNLJoin::operator==. I'd recommend making the code reuasable

nappelson · 2018-04-19T00:06:20Z

src/optimizer/plan_generator.cpp

+  vector<oid_t> left_keys;
+  vector<oid_t> right_keys;
+  for (auto &expr : op->left_keys) {
+    PELOTON_ASSERT(children_expr_map_[0].find(expr.get()) !=


it isn't immediately clear what is in children_expr_map_[0] vs [1]

It looks like this is how it is done in other parts of the code (not in your PR) but it may be worthwhile adding some documentation on why we only expect there to be two elements in this vector

nappelson · 2018-04-19T00:10:48Z

src/optimizer/plan_generator.cpp

+
+  vector<unique_ptr<const expression::AbstractExpression>> left_keys;
+  vector<unique_ptr<const expression::AbstractExpression>> right_keys;
+  vector<ExprMap> l_child_map{move(children_expr_map_[0])};


prefer left_child_map instead of l_child_map. generally good to use full words when possible (you wouldn't exceed the line length in this case)

Same can be applied to r_child_map

nappelson · 2018-04-19T00:12:40Z

src/optimizer/plan_generator.cpp

+  }
+  // Evaluate Expr for hash plan
+  vector<unique_ptr<const expression::AbstractExpression>> hash_keys;
+  for (auto &expr : op->right_keys) {


i'd imagine these can be const auto &expr based on what is going on here but could be wrong

nappelson · 2018-04-19T00:13:49Z