[35/47] incubator-quickstep git commit: Resolves SMA bugs revealed by TPCH. (#234)

[zu...@apache.org]

Resolves SMA bugs revealed by TPCH. (#234)


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

Branch: refs/heads/reorder-query-id-param
Commit: 882cd386ab184c69508a4d4ff71a64cca0053969
Parents: 0115e96
Author: Marc S <cr...@users.noreply.github.com>
Authored: Mon May 23 11:11:44 2016 -0700
Committer: Zuyu Zhang <zz...@pivotal.io>
Committed: Wed Jun 8 11:57:45 2016 -0700

----------------------------------------------------------------------
 storage/CMakeLists.txt                      |  1 +
 storage/SMAIndexSubBlock.cpp                | 55 +++++++++++++++++++-----
 storage/SMAIndexSubBlock.hpp                |  3 +-
 storage/tests/SMAIndexSubBlock_unittest.cpp | 52 ++++++++++++++++++++++
 4 files changed, 99 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/882cd386/storage/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/storage/CMakeLists.txt b/storage/CMakeLists.txt
index 26a3e32..a3093df 100644
--- a/storage/CMakeLists.txt
+++ b/storage/CMakeLists.txt
@@ -1451,6 +1451,7 @@ target_link_libraries(SMAIndexSubBlock_unittest
                       quickstep_types_CharType
                       quickstep_types_DoubleType
                       quickstep_types_FloatType
+                      quickstep_types_IntType
                       quickstep_types_LongType
                       quickstep_types_TypeFactory
                       quickstep_types_TypeID

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/882cd386/storage/SMAIndexSubBlock.cpp
----------------------------------------------------------------------
diff --git a/storage/SMAIndexSubBlock.cpp b/storage/SMAIndexSubBlock.cpp
index aa9bc54..3a1ccc2 100644
--- a/storage/SMAIndexSubBlock.cpp
+++ b/storage/SMAIndexSubBlock.cpp
@@ -181,19 +181,22 @@ Selectivity getSelectivity(const TypedValue &literal,
 }
 
 SMAPredicate* SMAPredicate::ExtractSMAPredicate(const ComparisonPredicate &predicate) {
-  if (predicate.getLeftOperand().hasStaticValue()) {
-    DLOG_IF(FATAL, predicate.getRightOperand().getDataSource() != Scalar::kAttribute);
+  // Check that the predicate is contains an attribute and a literal value.
+  if (predicate.getLeftOperand().hasStaticValue() &&
+      predicate.getRightOperand().getDataSource() == Scalar::kAttribute) {
     return new SMAPredicate(
          static_cast<const ScalarAttribute&>(predicate.getRightOperand()).getAttribute().getID(),
          flipComparisonID(predicate.getComparison().getComparisonID()),
          predicate.getLeftOperand().getStaticValue().makeReferenceToThis());
-  } else {
-    DLOG_IF(FATAL, predicate.getLeftOperand().getDataSource() != Scalar::kAttribute);
+  } else if (predicate.getRightOperand().hasStaticValue() &&
+      predicate.getLeftOperand().getDataSource() == Scalar::kAttribute) {
     return new SMAPredicate(
         static_cast<const ScalarAttribute&>(predicate.getLeftOperand()).getAttribute().getID(),
         predicate.getComparison().getComparisonID(),
         predicate.getRightOperand().getStaticValue().makeReferenceToThis());
   }
+  // Predicate is improper form, so return nullptr.
+  return nullptr;
 }
 
 /**
@@ -603,12 +606,47 @@ Selectivity SMAIndexSubBlock::getSelectivityForPredicate(const ComparisonPredica
   }
 
   std::unique_ptr<SMAPredicate> sma_predicate(SMAPredicate::ExtractSMAPredicate(predicate));
+  // The predicate did not contain a static value to compare against.
+  if (!sma_predicate) {
+    return Selectivity::kUnknown;
+  }
+
   const SMAEntry *entry = getEntryChecked(sma_predicate->attribute);
 
   // The attribute wasn't indexed.
   if (entry == nullptr || !entry->min_entry_ref.valid || !entry->max_entry_ref.valid) {
     return Selectivity::kUnknown;
   }
+
+  // Check that the type of the comparison is the same or can be coerced the
+  // type of the underlying attribute.
+  if (sma_predicate->literal.getTypeID() != entry->type_id) {
+    // Try to coerce the literal type to the entry type.
+    // It may have to account for variable length attributes.
+    int literal_length = -1;
+    if (TypeFactory::TypeRequiresLengthParameter(sma_predicate->literal.getTypeID())) {
+      literal_length = sma_predicate->literal.getAsciiStringLength();
+    }
+
+    const Type &literal_type = literal_length == -1 ?
+        TypeFactory::GetType(sma_predicate->literal.getTypeID(), false) :
+        TypeFactory::GetType(sma_predicate->literal.getTypeID(), static_cast<std::size_t>(literal_length), false);
+    const Type &attribute_type = literal_length == -1 ?
+        TypeFactory::GetType(entry->type_id, false) :
+        TypeFactory::GetType(entry->type_id, static_cast<std::size_t>(literal_length), false);
+    if (attribute_type.isSafelyCoercibleFrom(literal_type)) {
+      // Convert the literal's type inside the predicate.
+      SMAPredicate *replacement = new SMAPredicate(
+          sma_predicate->attribute,
+          sma_predicate->comparison,
+          attribute_type.coerceValue(sma_predicate->literal, literal_type));
+      sma_predicate.reset(replacement);
+    } else {
+      // The literal type cannot be converted, so do not evaluate with the SMA.
+      return Selectivity::kUnknown;
+    }
+  }
+
   return sma_internal::getSelectivity(
     sma_predicate->literal,
     sma_predicate->comparison,
@@ -623,12 +661,9 @@ predicate_cost_t SMAIndexSubBlock::estimatePredicateEvaluationCost(
   DCHECK(initialized_);
 
   // Check that at least one of the operands has a static value.
-  if (predicate.getLeftOperand().hasStaticValue() ||
-      predicate.getRightOperand().hasStaticValue()) {
-    Selectivity selectivity = getSelectivityForPredicate(predicate);
-    if (selectivity == Selectivity::kAll || selectivity == Selectivity::kNone) {
-      return predicate_cost::kConstantTime;
-    }
+  Selectivity selectivity = getSelectivityForPredicate(predicate);
+  if (selectivity == Selectivity::kAll || selectivity == Selectivity::kNone) {
+    return predicate_cost::kConstantTime;
   }
   return predicate_cost::kInfinite;
 }

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/882cd386/storage/SMAIndexSubBlock.hpp
----------------------------------------------------------------------
diff --git a/storage/SMAIndexSubBlock.hpp b/storage/SMAIndexSubBlock.hpp
index 71ca75b..895add9 100644
--- a/storage/SMAIndexSubBlock.hpp
+++ b/storage/SMAIndexSubBlock.hpp
@@ -120,7 +120,6 @@ struct SMAPredicate {
   // that the predicate had not been used previously.
   Selectivity selectivity;
 
- private:
   SMAPredicate(const attribute_id attr,
                const ComparisonID comp,
                const TypedValue lit)
@@ -376,7 +375,7 @@ class SMAIndexSubBlock : public IndexSubBlock {
  private:
   bool requiresRebuild() const;
 
-  inline sma_internal::Selectivity getSelectivityForPredicate(const ComparisonPredicate &predicate) const;
+  sma_internal::Selectivity getSelectivityForPredicate(const ComparisonPredicate &predicate) const;
 
   // Retrieves an entry, first checking if the given attribute is indexed.
   inline const sma_internal::SMAEntry* getEntryChecked(attribute_id attribute) const {

http://git-wip-us.apache.org/repos/asf/incubator-quickstep/blob/882cd386/storage/tests/SMAIndexSubBlock_unittest.cpp
----------------------------------------------------------------------
diff --git a/storage/tests/SMAIndexSubBlock_unittest.cpp b/storage/tests/SMAIndexSubBlock_unittest.cpp
index b709712..ad842c2 100644
--- a/storage/tests/SMAIndexSubBlock_unittest.cpp
+++ b/storage/tests/SMAIndexSubBlock_unittest.cpp
@@ -41,6 +41,7 @@
 #include "types/CharType.hpp"
 #include "types/DoubleType.hpp"
 #include "types/FloatType.hpp"
+#include "types/IntType.hpp"
 #include "types/LongType.hpp"
 #include "types/TypeFactory.hpp"
 #include "types/TypeID.hpp"
@@ -301,6 +302,10 @@ class SMAIndexSubBlockTest : public ::testing::Test {
     return !index_->header_->index_consistent;
   }
 
+  Selectivity getSelectivityForPredicate(const ComparisonPredicate &predicate) const {
+    return index_->getSelectivityForPredicate(predicate);
+  }
+
   std::unique_ptr<CatalogRelation> relation_;
   std::unique_ptr<MockTupleStorageSubBlock> tuple_store_;
   ScopedBuffer index_memory_;
@@ -359,6 +364,53 @@ TEST_F(SMAIndexSubBlockTest, TestConstructor) {
   EXPECT_FALSE(requiresRebuild());
 }
 
+TEST_F(SMAIndexSubBlockTest, TestGetSelectivityForPredicate) {
+  // Index long, float, bigchar, and varchar type.
+  createIndex({0, 2, 5, 6}, kIndexSubBlockSize);
+  int min = 0, max = 9010, step = 10;
+  for (std::size_t i = min; i <= static_cast<std::size_t>(max); i += step) {
+    generateAndInsertTuple(i, false, "suffix");
+  }
+  index_->rebuild();
+
+  // Tuple storage block now contains long values [0, 9010], and float values
+  // [0, 2252.5].
+  // Test several value types.
+  // Test with inline values, returning both possible value types.
+  std::unique_ptr<ComparisonPredicate> predicate(
+      generateNumericComparisonPredicate<LongType>(ComparisonID::kLess, 0, min));
+  Selectivity cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kNone, cost);
+
+  // Test on the float attribute with a type that will need coercion. In this
+  // case, a long type should not coerce to an integer type.
+  predicate.reset(generateNumericComparisonPredicate<LongType>(ComparisonID::kLess, 2, min));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kUnknown, cost);
+
+  predicate.reset(generateNumericComparisonPredicate<IntType>(ComparisonID::kLess, 2, min));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kNone, cost);
+
+  // This should be unknown because of the loss of precision on coercion.
+  predicate.reset(generateNumericComparisonPredicate<FloatType>(ComparisonID::kLess, 0, min + 0.5));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kUnknown, cost);
+
+  // Test coercion with String types as well.
+  predicate.reset(generateStringComparisonPredicate(ComparisonID::kLess, 6, ""));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kNone, cost);
+
+  predicate.reset(generateStringComparisonPredicate(ComparisonID::kLess, 5, ""));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kNone, cost);
+
+  predicate.reset(generateStringComparisonPredicate(ComparisonID::kLess, 5, "999999999999999"));
+  cost = getSelectivityForPredicate(*predicate);
+  EXPECT_EQ(Selectivity::kAll, cost);
+}
+
 TEST_F(SMAIndexSubBlockTest, TestRebuild) {
   // This test is checking to see if the min/max values are set correctly on
   // a rebuild of the index.