quickstep-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jianq...@apache.org
Subject [10/32] incubator-quickstep git commit: Fix the issues with the common subexpression feature
Date Thu, 04 May 2017 03:22:54 GMT
Fix the issues with the common subexpression feature


Project: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/commit/30021acf
Tree: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/tree/30021acf
Diff: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/diff/30021acf

Branch: refs/heads/new-op
Commit: 30021acf8bdf6d33f6e940f3343760384d971830
Parents: d6a01e7
Author: Jianqiao Zhu <jianqiao@cs.wisc.edu>
Authored: Mon Apr 24 02:33:39 2017 -0500
Committer: Zuyu Zhang <zuyuz@apache.org>
Committed: Mon Apr 24 17:55:22 2017 -0700

----------------------------------------------------------------------
 expressions/predicate/Predicate.hpp             |  6 +--
 expressions/scalar/Scalar.cpp                   |  2 +-
 expressions/scalar/Scalar.hpp                   |  5 +-
 expressions/scalar/ScalarSharedExpression.cpp   | 51 +++++++++-----------
 expressions/scalar/ScalarSharedExpression.hpp   | 13 +++--
 .../tests/ScalarCaseExpression_unittest.cpp     | 26 +++++++---
 .../expressions/CommonSubexpression.hpp         |  4 +-
 query_optimizer/expressions/SimpleCase.cpp      |  6 +--
 query_optimizer/rules/CollapseSelection.cpp     |  2 +-
 query_optimizer/rules/CollapseSelection.hpp     |  2 +
 .../rules/ExtractCommonSubexpression.cpp        |  2 +-
 .../rules/ExtractCommonSubexpression.hpp        |  2 +-
 .../rules/ReuseAggregateExpressions.cpp         |  7 +--
 .../rules/ReuseAggregateExpressions.hpp         |  2 +
 14 files changed, 68 insertions(+), 62 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/predicate/Predicate.hpp
----------------------------------------------------------------------
diff --git a/expressions/predicate/Predicate.hpp b/expressions/predicate/Predicate.hpp
index 6a2ba6d..df04644 100644
--- a/expressions/predicate/Predicate.hpp
+++ b/expressions/predicate/Predicate.hpp
@@ -65,11 +65,7 @@ class Predicate : public Expression {
    **/
   static const char *kPredicateTypeNames[kNumPredicateTypes];
 
-  /**
-   * @brief Virtual destructor.
-   *
-   **/
-  virtual ~Predicate() {
+  ~Predicate() override {
   }
 
   std::string getName() const override {

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/scalar/Scalar.cpp
----------------------------------------------------------------------
diff --git a/expressions/scalar/Scalar.cpp b/expressions/scalar/Scalar.cpp
index c552d8b..da0fc1b 100644
--- a/expressions/scalar/Scalar.cpp
+++ b/expressions/scalar/Scalar.cpp
@@ -31,8 +31,8 @@ const char *Scalar::kScalarDataSourceNames[] = {
   "Attribute",
   "UnaryExpression",
   "BinaryExpression",
-  "SharedExpression",
   "SimpleCase"
+  "SharedExpression",
 };
 
 const TypedValue& Scalar::getStaticValue() const {

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/scalar/Scalar.hpp
----------------------------------------------------------------------
diff --git a/expressions/scalar/Scalar.hpp b/expressions/scalar/Scalar.hpp
index 472b71c..6e482c2 100644
--- a/expressions/scalar/Scalar.hpp
+++ b/expressions/scalar/Scalar.hpp
@@ -69,10 +69,7 @@ class Scalar : public Expression {
    **/
   static const char *kScalarDataSourceNames[kNumScalarDataSources];
 
-  /**
-   * @brief Virtual destructor.
-   **/
-  virtual ~Scalar() {
+  ~Scalar() override {
   }
 
   std::string getName() const override {

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/scalar/ScalarSharedExpression.cpp
----------------------------------------------------------------------
diff --git a/expressions/scalar/ScalarSharedExpression.cpp b/expressions/scalar/ScalarSharedExpression.cpp
index f97c60b..e301116 100644
--- a/expressions/scalar/ScalarSharedExpression.cpp
+++ b/expressions/scalar/ScalarSharedExpression.cpp
@@ -34,19 +34,12 @@ namespace quickstep {
 
 struct SubBlocksReference;
 
-ScalarSharedExpression::ScalarSharedExpression(const int share_id,
-                                               Scalar *operand)
-    : Scalar(operand->getType()),
-      share_id_(share_id),
-      operand_(operand) {
-}
-
 serialization::Scalar ScalarSharedExpression::getProto() const {
   serialization::Scalar proto;
   proto.set_data_source(serialization::Scalar::SHARED_EXPRESSION);
   proto.SetExtension(serialization::ScalarSharedExpression::share_id, share_id_);
   proto.MutableExtension(serialization::ScalarSharedExpression::operand)
-      ->CopyFrom(operand_->getProto());
+      ->MergeFrom(operand_->getProto());
 
   return proto;
 }
@@ -81,16 +74,16 @@ ColumnVectorPtr ScalarSharedExpression::getAllValues(
     ColumnVectorCache *cv_cache) const {
   if (cv_cache == nullptr) {
     return operand_->getAllValues(accessor, sub_blocks_ref, cv_cache);
+  }
+
+  ColumnVectorPtr result;
+  if (cv_cache->contains(share_id_)) {
+    result = cv_cache->get(share_id_);
   } else {
-    ColumnVectorPtr result;
-    if (cv_cache->contains(share_id_)) {
-      result = cv_cache->get(share_id_);
-    } else {
-      result = operand_->getAllValues(accessor, sub_blocks_ref, cv_cache);
-      cv_cache->set(share_id_, result);
-    }
-    return result;
+    result = operand_->getAllValues(accessor, sub_blocks_ref, cv_cache);
+    cv_cache->set(share_id_, result);
   }
+  return result;
 }
 
 ColumnVectorPtr ScalarSharedExpression::getAllValuesForJoin(
@@ -107,21 +100,21 @@ ColumnVectorPtr ScalarSharedExpression::getAllValuesForJoin(
                                          right_accessor,
                                          joined_tuple_ids,
                                          cv_cache);
+  }
+
+  ColumnVectorPtr result;
+  if (cv_cache->contains(share_id_)) {
+    result = cv_cache->get(share_id_);
   } else {
-    ColumnVectorPtr result;
-    if (cv_cache->contains(share_id_)) {
-      result = cv_cache->get(share_id_);
-    } else {
-      result = operand_->getAllValuesForJoin(left_relation_id,
-                                             left_accessor,
-                                             right_relation_id,
-                                             right_accessor,
-                                             joined_tuple_ids,
-                                             cv_cache);
-      cv_cache->set(share_id_, result);
-    }
-    return result;
+    result = operand_->getAllValuesForJoin(left_relation_id,
+                                           left_accessor,
+                                           right_relation_id,
+                                           right_accessor,
+                                           joined_tuple_ids,
+                                           cv_cache);
+    cv_cache->set(share_id_, result);
   }
+  return result;
 }
 
 void ScalarSharedExpression::getFieldStringItems(

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/scalar/ScalarSharedExpression.hpp
----------------------------------------------------------------------
diff --git a/expressions/scalar/ScalarSharedExpression.hpp b/expressions/scalar/ScalarSharedExpression.hpp
index d5dddbc..f39c45b 100644
--- a/expressions/scalar/ScalarSharedExpression.hpp
+++ b/expressions/scalar/ScalarSharedExpression.hpp
@@ -53,11 +53,16 @@ class ScalarSharedExpression : public Scalar {
   /**
    * @brief Constructor.
    *
-   * @param share_id The unique integer identifier for each equivalence class of
-   *        common subexpressions.
-   * @param operand The underlying scalar subexpression.
+   * @param share_id The unique integer identifier for each equivalence class
+   *        of common subexpressions.
+   * @param operand The underlying scalar subexpression, which this
+   *        ScalarSharedExpression takes ownership of.
    **/
-  ScalarSharedExpression(const int share_id, Scalar *operand);
+  ScalarSharedExpression(const int share_id, Scalar *operand)
+      : Scalar(operand->getType()),
+        share_id_(share_id),
+        operand_(operand) {
+  }
 
   /**
    * @brief Destructor.

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/expressions/scalar/tests/ScalarCaseExpression_unittest.cpp
----------------------------------------------------------------------
diff --git a/expressions/scalar/tests/ScalarCaseExpression_unittest.cpp b/expressions/scalar/tests/ScalarCaseExpression_unittest.cpp
index 2de9e84..7182642 100644
--- a/expressions/scalar/tests/ScalarCaseExpression_unittest.cpp
+++ b/expressions/scalar/tests/ScalarCaseExpression_unittest.cpp
@@ -309,7 +309,9 @@ TEST_F(ScalarCaseExpressionTest, BasicComparisonAndLiteralWithFilteredInputTest)
                         varchar_type));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(filtered_accessor.get(), nullptr, nullptr));
+      case_expr.getAllValues(filtered_accessor.get(),
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_FALSE(result_cv->isNative());
   const IndirectColumnVector &indirect_result_cv
       = static_cast<const IndirectColumnVector&>(*result_cv);
@@ -381,7 +383,9 @@ TEST_F(ScalarCaseExpressionTest, WhenClauseOrderTest) {
                         varchar_type));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(&sample_data_value_accessor_, nullptr, nullptr));
+      case_expr.getAllValues(&sample_data_value_accessor_,
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_FALSE(result_cv->isNative());
   const IndirectColumnVector &indirect_result_cv
       = static_cast<const IndirectColumnVector&>(*result_cv);
@@ -475,7 +479,9 @@ TEST_F(ScalarCaseExpressionTest, ComplexConjunctionAndCalculatedExpressionTest)
           new ScalarAttribute(*sample_relation_->getAttributeById(0))));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(&sample_data_value_accessor_, nullptr, nullptr));
+      case_expr.getAllValues(&sample_data_value_accessor_,
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_TRUE(result_cv->isNative());
   const NativeColumnVector &native_result_cv
       = static_cast<const NativeColumnVector&>(*result_cv);
@@ -598,7 +604,9 @@ TEST_F(ScalarCaseExpressionTest,
           new ScalarAttribute(*sample_relation_->getAttributeById(0))));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(filtered_accessor.get(), nullptr, nullptr));
+      case_expr.getAllValues(filtered_accessor.get(),
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_TRUE(result_cv->isNative());
   const NativeColumnVector &native_result_cv
       = static_cast<const NativeColumnVector&>(*result_cv);
@@ -708,7 +716,9 @@ TEST_F(ScalarCaseExpressionTest, ComplexDisjunctionAndCalculatedExpressionTest)
           new ScalarAttribute(*sample_relation_->getAttributeById(0))));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(&sample_data_value_accessor_, nullptr, nullptr));
+      case_expr.getAllValues(&sample_data_value_accessor_,
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_TRUE(result_cv->isNative());
   const NativeColumnVector &native_result_cv
       = static_cast<const NativeColumnVector&>(*result_cv);
@@ -828,7 +838,9 @@ TEST_F(ScalarCaseExpressionTest,
           new ScalarAttribute(*sample_relation_->getAttributeById(0))));
 
   ColumnVectorPtr result_cv(
-      case_expr.getAllValues(filtered_accessor.get(), nullptr, nullptr));
+      case_expr.getAllValues(filtered_accessor.get(),
+                             nullptr /* sub_blocks_ref */,
+                             nullptr /* cv_cache */));
   ASSERT_TRUE(result_cv->isNative());
   const NativeColumnVector &native_result_cv
       = static_cast<const NativeColumnVector&>(*result_cv);
@@ -935,7 +947,7 @@ TEST_F(ScalarCaseExpressionTest, JoinTest) {
       1,
       &other_accessor,
       joined_tuple_ids,
-      nullptr));
+      nullptr /* cv_cache */));
   ASSERT_TRUE(result_cv->isNative());
   const NativeColumnVector &native_result_cv
       = static_cast<const NativeColumnVector&>(*result_cv);

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/expressions/CommonSubexpression.hpp
----------------------------------------------------------------------
diff --git a/query_optimizer/expressions/CommonSubexpression.hpp b/query_optimizer/expressions/CommonSubexpression.hpp
index ce7589d..2c2d86c 100644
--- a/query_optimizer/expressions/CommonSubexpression.hpp
+++ b/query_optimizer/expressions/CommonSubexpression.hpp
@@ -126,8 +126,8 @@ class CommonSubexpression : public Scalar {
     addChild(operand);
   }
 
-  ExprId common_subexpression_id_;
-  ScalarPtr operand_;
+  const ExprId common_subexpression_id_;
+  const ScalarPtr operand_;
 
   DISALLOW_COPY_AND_ASSIGN(CommonSubexpression);
 };

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/expressions/SimpleCase.cpp
----------------------------------------------------------------------
diff --git a/query_optimizer/expressions/SimpleCase.cpp b/query_optimizer/expressions/SimpleCase.cpp
index ccdd8e5..b127d51 100644
--- a/query_optimizer/expressions/SimpleCase.cpp
+++ b/query_optimizer/expressions/SimpleCase.cpp
@@ -195,10 +195,8 @@ bool SimpleCase::equals(const ScalarPtr &other) const {
       return false;
     }
   }
-  if ((else_result_expression_ == nullptr
-       || expr->else_result_expression_ == nullptr)
-      && else_result_expression_ != expr->else_result_expression_) {
-    return false;
+  if (else_result_expression_ == nullptr || expr->else_result_expression_ == nullptr)
{
+    return else_result_expression_ == expr->else_result_expression_;
   }
   if (!else_result_expression_->equals(expr->else_result_expression_)) {
     return false;

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/CollapseSelection.cpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/CollapseSelection.cpp b/query_optimizer/rules/CollapseSelection.cpp
index e5427b4..f92e1b2 100644
--- a/query_optimizer/rules/CollapseSelection.cpp
+++ b/query_optimizer/rules/CollapseSelection.cpp
@@ -46,7 +46,7 @@ P::PhysicalPtr CollapseSelection::applyToNode(const P::PhysicalPtr &input)
{
         selection->project_expressions();
     PullUpProjectExpressions(child_selection->project_expressions(),
                              {} /* non_project_expression_lists */,
-                             {&project_expressions} /* project_expression_lists */);
+                             { &project_expressions } /* project_expression_lists */);
     return P::Selection::Create(child_selection->input(),
                                 project_expressions,
                                 selection->filter_predicate());

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/CollapseSelection.hpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/CollapseSelection.hpp b/query_optimizer/rules/CollapseSelection.hpp
index bc5e4a3..25c3492 100644
--- a/query_optimizer/rules/CollapseSelection.hpp
+++ b/query_optimizer/rules/CollapseSelection.hpp
@@ -43,6 +43,8 @@ class CollapseSelection : public BottomUpRule<physical::Physical>
{
    */
   CollapseSelection() {}
 
+  ~CollapseSelection() override {}
+
   std::string getName() const override {
     return "CollapseSelection";
   }

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/ExtractCommonSubexpression.cpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/ExtractCommonSubexpression.cpp b/query_optimizer/rules/ExtractCommonSubexpression.cpp
index e3f996c..63b6b17 100644
--- a/query_optimizer/rules/ExtractCommonSubexpression.cpp
+++ b/query_optimizer/rules/ExtractCommonSubexpression.cpp
@@ -258,7 +258,7 @@ E::ExpressionPtr ExtractCommonSubexpression::transformExpression(
 bool ExtractCommonSubexpression::visitAndCount(
     const E::ExpressionPtr &expression,
     ScalarCounter *counter,
-    ScalarHashable *hashable) {
+    ScalarHashable *hashable) const {
   // This bool flag is for avoiding some unnecessary hash() computation.
   bool children_hashable = true;
 

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/ExtractCommonSubexpression.hpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/ExtractCommonSubexpression.hpp b/query_optimizer/rules/ExtractCommonSubexpression.hpp
index 3cdd70e..26b09cc 100644
--- a/query_optimizer/rules/ExtractCommonSubexpression.hpp
+++ b/query_optimizer/rules/ExtractCommonSubexpression.hpp
@@ -111,7 +111,7 @@ class ExtractCommonSubexpression : public Rule<physical::Physical>
{
   bool visitAndCount(
       const expressions::ExpressionPtr &expression,
       ScalarCounter *counter,
-      ScalarHashable *hashable);
+      ScalarHashable *hashable) const;
 
   // Traverse the expression tree and transform subexpressions (to common
   // subexpressions) if applicable.

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/ReuseAggregateExpressions.cpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/ReuseAggregateExpressions.cpp b/query_optimizer/rules/ReuseAggregateExpressions.cpp
index 79dede6..a7c62c6 100644
--- a/query_optimizer/rules/ReuseAggregateExpressions.cpp
+++ b/query_optimizer/rules/ReuseAggregateExpressions.cpp
@@ -157,7 +157,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
     count_star_ref = *it;
 
     for (++it; it != count_star_info.end(); ++it) {
-      agg_refs[*it].reset(new AggregateReference(count_star_ref));
+      agg_refs[*it] = std::make_unique<AggregateReference>(count_star_ref);
     }
   }
 
@@ -194,7 +194,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
             sum_it == ref_map.end() ? kInvalidRef : sum_it->second.front();
 
         for (const std::size_t idx : avg_it->second) {
-          agg_refs[idx].reset(new AggregateReference(sum_ref, count_ref));
+          agg_refs[idx] = std::make_unique<AggregateReference>(sum_ref, count_ref);
         }
         is_avg_processed = true;
       }
@@ -209,7 +209,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
       DCHECK(!indices.empty());
       const std::size_t ref = indices.front();
       for (std::size_t i = 1; i < indices.size(); ++i) {
-        agg_refs[indices[i]].reset(new AggregateReference(ref));
+        agg_refs[indices[i]] = std::make_unique<AggregateReference>(ref);
       }
     }
   }
@@ -329,6 +329,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
                                agg_expr->attribute_name(),
                                agg_expr->attribute_alias(),
                                agg_expr->relation_name()));
+          break;
         }
       }
     }

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/30021acf/query_optimizer/rules/ReuseAggregateExpressions.hpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/ReuseAggregateExpressions.hpp b/query_optimizer/rules/ReuseAggregateExpressions.hpp
index 182e9d9..12f81d6 100644
--- a/query_optimizer/rules/ReuseAggregateExpressions.hpp
+++ b/query_optimizer/rules/ReuseAggregateExpressions.hpp
@@ -98,6 +98,8 @@ class ReuseAggregateExpressions : public BottomUpRule<physical::Physical>
{
   explicit ReuseAggregateExpressions(OptimizerContext *optimizer_context)
       : optimizer_context_(optimizer_context) {}
 
+  ~ReuseAggregateExpressions() override {}
+
   std::string getName() const override {
     return "ReuseAggregateExpressions";
   }


Mime
View raw message