You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@quickstep.apache.org by ji...@apache.org on 2017/04/24 07:33:59 UTC
incubator-quickstep git commit: Fix the issues with the common
subexpression feature
Repository: incubator-quickstep
Updated Branches:
refs/heads/fix-common-subexpression [created] 6a31bf96c
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/6a31bf96
Tree: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/tree/6a31bf96
Diff: http://git-wip-us.apache.org/repos/asf/incubator-quickstep/diff/6a31bf96
Branch: refs/heads/fix-common-subexpression
Commit: 6a31bf96cb4fd1a99a9506426145d27ddc333421
Parents: 8169306
Author: Jianqiao Zhu <ji...@cs.wisc.edu>
Authored: Mon Apr 24 02:33:39 2017 -0500
Committer: Jianqiao Zhu <ji...@cs.wisc.edu>
Committed: Mon Apr 24 02:33:39 2017 -0500
----------------------------------------------------------------------
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 | 1 +
.../rules/ReuseAggregateExpressions.hpp | 2 +
14 files changed, 65 insertions(+), 59 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/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/6a31bf96/query_optimizer/rules/ReuseAggregateExpressions.cpp
----------------------------------------------------------------------
diff --git a/query_optimizer/rules/ReuseAggregateExpressions.cpp b/query_optimizer/rules/ReuseAggregateExpressions.cpp
index 79dede6..1d68027 100644
--- a/query_optimizer/rules/ReuseAggregateExpressions.cpp
+++ b/query_optimizer/rules/ReuseAggregateExpressions.cpp
@@ -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/6a31bf96/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";
}