You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by zuyu <gi...@git.apache.org> on 2017/03/18 11:54:23 UTC
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
GitHub user zuyu opened a pull request:
https://github.com/apache/incubator-quickstep/pull/217
QUICKSTEP-73: Multi-attribute PartitionSchemeHeader.
This PR added multi-attribute partitions support.
```
CREATE TABLE foo5 (attr1 INT, attr2 LONG, attr3 FLOAT, attr4 DOUBLE, attr5 CHAR(5),
attr6 VARCHAR(4))
PARTITION BY HASH(attr1, attr2, attr3, attr4, attr5, attr6) PARTITIONS 4;
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zuyu/incubator-quickstep multi-attr-partition
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/217.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #217
----
commit 02d0a3b5b6676ffb1ea7cc3d19b232291d8f6323
Author: Zuyu Zhang <zu...@apache.org>
Date: 2017-03-18T10:28:38Z
QUICKSTEP-73: Multi-attribute PartitionSchemeHeader.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r108021399
--- Diff: query_optimizer/resolver/Resolver.cpp ---
@@ -724,6 +708,25 @@ const S::PartitionSchemeHeader* Resolver::resolvePartitionClause(
THROW_SQL_ERROR_AT(partition_type_string) << "Unrecognized partition type: " << partition_type;
}
+ proto->set_num_partitions(partition_clause->num_partitions()->long_value());
--- End diff --
Both cases are covered in the parser, not in the optimizer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #217: QUICKSTEP-73: Multi-attribute PartitionSchem...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/217
Hi @hbdeshmukh,
I have reported QUICKSTEP-86 for the test failure, and squashed all the commits. Please merge it. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107975596
--- Diff: query_optimizer/resolver/Resolver.cpp ---
@@ -724,6 +708,25 @@ const S::PartitionSchemeHeader* Resolver::resolvePartitionClause(
THROW_SQL_ERROR_AT(partition_type_string) << "Unrecognized partition type: " << partition_type;
}
+ proto->set_num_partitions(partition_clause->num_partitions()->long_value());
+
+ std::unordered_set<attribute_id> unique_partition_attrs;
+ for (const ParseString &partition_attribute_name : partition_clause->attribute_name_list()) {
+ const attribute_id attr_id = GetAttributeIdFromName(create_table_statement.attribute_definition_list(),
+ partition_attribute_name.value());
+ if (attr_id == kInvalidAttributeID) {
+ THROW_SQL_ERROR_AT(&partition_attribute_name)
+ << "The given attribute was not found.";
+ } else if (unique_partition_attrs.count(attr_id)) {
--- End diff --
For readability, can we change this to ``count > 0`` or use ``find`` instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #217: QUICKSTEP-73: Multi-attribute PartitionSchem...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/217
Hi @zuyu
Looks good to me. I just have one last comment about the commented code in unit tests. After that, we should be good to go. Also, there are two commits for this PR, would you mind squashing them to one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #217: QUICKSTEP-73: Multi-attribute PartitionSchem...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/217
Hi @zuyu
Looks like a compilation error is causing travis to fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107739146
--- Diff: catalog/PartitionSchemeHeader.cpp ---
@@ -32,16 +34,29 @@
#include "glog/logging.h"
+using std::move;
+using std::size_t;
+using std::vector;
+
namespace quickstep {
PartitionSchemeHeader::PartitionSchemeHeader(const PartitionType type,
const std::size_t num_partitions,
- const attribute_id attr_id)
+ PartitionAttributeIds &&attr_ids) // NOLINT(whitespace/operators)
: partition_type_(type),
num_partitions_(num_partitions),
- partition_attribute_id_(attr_id) {
+ partition_attribute_ids_(move(attr_ids)) {
DCHECK_GT(num_partitions, 0u);
- DCHECK_GE(attr_id, 0);
+
+#ifdef QUICKSTEP_DEBUG
--- End diff --
Optional, but a couple of lines regarding what you are trying to do here should be helpful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #217: QUICKSTEP-73: Multi-attribute PartitionSchem...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/217
Hi @hbdeshmukh,
Thank you for the review. I have addressed most of issues, and please take another look.
And I also would like to propose to remove the debug builds in Travis CI.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107966844
--- Diff: catalog/PartitionSchemeHeader.cpp ---
@@ -77,32 +95,44 @@ PartitionSchemeHeader* PartitionSchemeHeader::ReconstructFromProto(
<< "Attempted to create PartitionSchemeHeader from an invalid proto description:\n"
<< proto.DebugString();
+ PartitionAttributeIds partition_attribute_ids;
+ for (int i = 0; i < proto.partition_attribute_ids_size(); ++i) {
+ partition_attribute_ids.push_back(proto.partition_attribute_ids(i));
+ }
+
switch (proto.partition_type()) {
case serialization::PartitionSchemeHeader::HASH: {
- return new HashPartitionSchemeHeader(proto.num_partitions(), proto.partition_attribute_id());
+ return new HashPartitionSchemeHeader(proto.num_partitions(), move(partition_attribute_ids));
}
case serialization::PartitionSchemeHeader::RANGE: {
- const Type &attr_type =
- TypeFactory::ReconstructFromProto(proto.GetExtension(
- serialization::RangePartitionSchemeHeader::partition_attr_type));
-
- std::vector<TypedValue> partition_ranges;
- for (int i = 0;
- i < proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_range_boundaries);
- ++i) {
- partition_ranges.emplace_back(
- TypedValue::ReconstructFromProto(
- proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_range_boundaries, i)));
+ std::vector<const Type*> attr_types;
+ for (int i = 0; i < proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_attr_types); ++i) {
+ attr_types.push_back(&TypeFactory::ReconstructFromProto(
+ proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_attr_types, i)));
+ }
+
+ const int partition_ranges_size =
+ proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_range_boundaries);
+ std::vector<PartitionValues> partition_ranges(partition_ranges_size);
+ for (int i = 0; i < partition_ranges_size; ++i) {
+ const auto &proto_partition_values =
+ proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_range_boundaries, i);
+ for (int j = 0; j < proto_partition_values.partition_values_size(); ++j) {
+ partition_ranges[i].emplace_back(TypedValue::ReconstructFromProto(
+ proto_partition_values.partition_values(j)));
+ }
}
- return new RangePartitionSchemeHeader(attr_type,
- proto.num_partitions(),
- proto.partition_attribute_id(),
- std::move(partition_ranges));
+ return new RangePartitionSchemeHeader(proto.num_partitions(),
+ move(partition_attribute_ids),
+ move(attr_types),
+ move(partition_ranges));
}
default:
LOG(FATAL) << "Invalid partition scheme header.";
}
+
+ return nullptr;
--- End diff --
In [Travis CI](https://travis-ci.org/apache/incubator-quickstep/jobs/212429450#L1226), the compiler complains `-Werror=return-type` here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r108021509
--- Diff: catalog/PartitionSchemeHeader.cpp ---
@@ -32,16 +34,29 @@
#include "glog/logging.h"
+using std::move;
+using std::size_t;
+using std::vector;
+
namespace quickstep {
PartitionSchemeHeader::PartitionSchemeHeader(const PartitionType type,
const std::size_t num_partitions,
- const attribute_id attr_id)
+ PartitionAttributeIds &&attr_ids) // NOLINT(whitespace/operators)
: partition_type_(type),
num_partitions_(num_partitions),
- partition_attribute_id_(attr_id) {
+ partition_attribute_ids_(move(attr_ids)) {
DCHECK_GT(num_partitions, 0u);
- DCHECK_GE(attr_id, 0);
+
+#ifdef QUICKSTEP_DEBUG
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107977156
--- Diff: storage/InsertDestination.cpp ---
@@ -682,8 +685,11 @@ void PartitionAwareInsertDestination::bulkInsertTuplesWithRemappedAttributes(
// set a bit in the appropriate TupleIdSequence.
accessor->beginIteration();
while (accessor->next()) {
- TypedValue attr_val = accessor->getTypedValue(attribute_map[partition_attribute_id]);
- partition_membership[partition_scheme_header_->getPartitionId(attr_val)]
+ PartitionSchemeHeader::PartitionValues values;
+ for (const attribute_id attr_id : partition_scheme_header_->getPartitionAttributeIds()) {
+ values.push_back(accessor->getTypedValue(attr_id));
+ }
--- End diff --
Did you take a look at the function ``getTupleWithAttributes()``. It may be useful here. Moreover the ``tuple`` class can compute hash using the ``getTupleHash`` function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107948593
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -180,16 +190,17 @@ class HashPartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Calulate the partition id into which the attribute value
* should be inserted.
*
- * @param value_of_attribute The attribute value for which the
- * partition id is to be determined.
+ * @param value_of_attributes The attribute value for which the
+ * partition id is to be determined.
* @return The partition id of the partition for the attribute value.
**/
partition_id getPartitionId(
- const TypedValue &value_of_attribute) const override {
+ const PartitionValues &value_of_attributes) const override {
--- End diff --
In the type definition of ``PartitionValues``, you said it is for representing the boundary for partition values. That doesn't seem true here. Why not just use the vector of ``TypedValue`` instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107949434
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -249,20 +269,22 @@ class RangePartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Calulate the partition id into which the attribute value
* should be inserted.
*
- * @param value_of_attribute The attribute value for which the
- * partition id is to be determined.
+ * @param value_of_attributes The attribute value for which the
--- End diff --
Nitpick - values instead of value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107951792
--- Diff: catalog/tests/PartitionScheme_unittest.cpp ---
@@ -343,15 +363,43 @@ TEST(PartitionSchemeHeaderTest, VarCharRangePartitionSchemeHeaderTest) {
for (size_t i = 0; i < num_strings; ++i) {
EXPECT_EQ(kExpectedPartitions[i],
partition_scheme_header->getPartitionId(
- TypedValue(kVarChar,
- kSampleStrings[i],
- std::strlen(kSampleStrings[i]) + 1)));
+ { TypedValue(kVarChar, kSampleStrings[i], std::strlen(kSampleStrings[i]) + 1) }));
}
}
+TEST(PartitionSchemeHeaderTest, MultiAttributeRangePartitionSchemeHeaderTest) {
+ vector<PartitionSchemeHeader::PartitionValues> partition_ranges;
+ // Partition boundaries are { 0, 0.00000 }, { 10, 10.0000 }, { 20, 20.00000 }
+ // Last partition can hold upto infinity.
+ // First partition can hold from { -infinity, -infinity } to { 0, -2^(-1074) }.
--- End diff --
Can you explain -2^(-1074)? I thought the first partition should be < {0, 0.0000} and > {-Inf, -Inf}.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107982736
--- Diff: storage/InsertDestination.cpp ---
@@ -682,8 +685,11 @@ void PartitionAwareInsertDestination::bulkInsertTuplesWithRemappedAttributes(
// set a bit in the appropriate TupleIdSequence.
accessor->beginIteration();
while (accessor->next()) {
- TypedValue attr_val = accessor->getTypedValue(attribute_map[partition_attribute_id]);
- partition_membership[partition_scheme_header_->getPartitionId(attr_val)]
+ PartitionSchemeHeader::PartitionValues values;
+ for (const attribute_id attr_id : partition_scheme_header_->getPartitionAttributeIds()) {
+ values.push_back(accessor->getTypedValue(attr_id));
+ }
--- End diff --
I just checked the methods you mentioned, but I don't think we could use them, as we also need to get partition id in a range partition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107947951
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -180,16 +190,17 @@ class HashPartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Calulate the partition id into which the attribute value
* should be inserted.
*
- * @param value_of_attribute The attribute value for which the
- * partition id is to be determined.
+ * @param value_of_attributes The attribute value for which the
--- End diff --
Nitpick - attribute values instead of attribute value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107980470
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -288,22 +310,46 @@ class RangePartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Check if the partition range boundaries are in ascending order.
**/
void checkPartitionRangeBoundaries() {
+ for (const PartitionValues &partition_range_boundary : partition_range_boundaries_) {
+ CHECK_EQ(partition_attribute_ids_.size(), partition_range_boundary.size())
+ << "A partition boundary has different size than that of partition attributes.";
+ }
+
for (partition_id part_id = 1; part_id < partition_range_boundaries_.size(); ++part_id) {
- if (less_unchecked_comparator_->compareTypedValues(
- partition_range_boundaries_[part_id],
- partition_range_boundaries_[part_id - 1])) {
- LOG(FATAL) << "Partition boundaries are not in ascending order.";
+ CHECK(lessThan(partition_range_boundaries_[part_id - 1], partition_range_boundaries_[part_id]))
+ << "Partition boundaries are not in ascending order.";
+ }
+ }
+
+ bool lessThan(const PartitionValues &lhs, const PartitionValues &rhs) const {
+ DCHECK_EQ(partition_attribute_ids_.size(), lhs.size());
+ DCHECK_EQ(partition_attribute_ids_.size(), rhs.size());
+
+ for (std::size_t attr_index = 0; attr_index < partition_attribute_ids_.size(); ++attr_index) {
+ if (less_unchecked_comparators_[attr_index]->compareTypedValues(lhs[attr_index], rhs[attr_index])) {
+ break;
--- End diff --
We use [lexicographical order](https://en.wikipedia.org/wiki/Lexicographical_order) here, `Given two different sequences of the same length, a1a2...ak and b1b2...bk, the first one is smaller than the second one for the lexicographical order, if ai<bi (for the order of A), for the first i where ai and bi differ.`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107946806
--- Diff: catalog/PartitionSchemeHeader.cpp ---
@@ -77,32 +95,44 @@ PartitionSchemeHeader* PartitionSchemeHeader::ReconstructFromProto(
<< "Attempted to create PartitionSchemeHeader from an invalid proto description:\n"
<< proto.DebugString();
+ PartitionAttributeIds partition_attribute_ids;
+ for (int i = 0; i < proto.partition_attribute_ids_size(); ++i) {
+ partition_attribute_ids.push_back(proto.partition_attribute_ids(i));
+ }
+
switch (proto.partition_type()) {
case serialization::PartitionSchemeHeader::HASH: {
- return new HashPartitionSchemeHeader(proto.num_partitions(), proto.partition_attribute_id());
+ return new HashPartitionSchemeHeader(proto.num_partitions(), move(partition_attribute_ids));
}
case serialization::PartitionSchemeHeader::RANGE: {
- const Type &attr_type =
- TypeFactory::ReconstructFromProto(proto.GetExtension(
- serialization::RangePartitionSchemeHeader::partition_attr_type));
-
- std::vector<TypedValue> partition_ranges;
- for (int i = 0;
- i < proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_range_boundaries);
- ++i) {
- partition_ranges.emplace_back(
- TypedValue::ReconstructFromProto(
- proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_range_boundaries, i)));
+ std::vector<const Type*> attr_types;
+ for (int i = 0; i < proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_attr_types); ++i) {
+ attr_types.push_back(&TypeFactory::ReconstructFromProto(
+ proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_attr_types, i)));
+ }
+
+ const int partition_ranges_size =
+ proto.ExtensionSize(serialization::RangePartitionSchemeHeader::partition_range_boundaries);
+ std::vector<PartitionValues> partition_ranges(partition_ranges_size);
+ for (int i = 0; i < partition_ranges_size; ++i) {
+ const auto &proto_partition_values =
+ proto.GetExtension(serialization::RangePartitionSchemeHeader::partition_range_boundaries, i);
+ for (int j = 0; j < proto_partition_values.partition_values_size(); ++j) {
+ partition_ranges[i].emplace_back(TypedValue::ReconstructFromProto(
+ proto_partition_values.partition_values(j)));
+ }
}
- return new RangePartitionSchemeHeader(attr_type,
- proto.num_partitions(),
- proto.partition_attribute_id(),
- std::move(partition_ranges));
+ return new RangePartitionSchemeHeader(proto.num_partitions(),
+ move(partition_attribute_ids),
+ move(attr_types),
+ move(partition_ranges));
}
default:
LOG(FATAL) << "Invalid partition scheme header.";
}
+
+ return nullptr;
--- End diff --
Isn't this return statement redundant?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107950590
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -288,22 +310,46 @@ class RangePartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Check if the partition range boundaries are in ascending order.
**/
void checkPartitionRangeBoundaries() {
+ for (const PartitionValues &partition_range_boundary : partition_range_boundaries_) {
+ CHECK_EQ(partition_attribute_ids_.size(), partition_range_boundary.size())
+ << "A partition boundary has different size than that of partition attributes.";
+ }
+
for (partition_id part_id = 1; part_id < partition_range_boundaries_.size(); ++part_id) {
- if (less_unchecked_comparator_->compareTypedValues(
- partition_range_boundaries_[part_id],
- partition_range_boundaries_[part_id - 1])) {
- LOG(FATAL) << "Partition boundaries are not in ascending order.";
+ CHECK(lessThan(partition_range_boundaries_[part_id - 1], partition_range_boundaries_[part_id]))
+ << "Partition boundaries are not in ascending order.";
+ }
+ }
+
+ bool lessThan(const PartitionValues &lhs, const PartitionValues &rhs) const {
+ DCHECK_EQ(partition_attribute_ids_.size(), lhs.size());
+ DCHECK_EQ(partition_attribute_ids_.size(), rhs.size());
+
+ for (std::size_t attr_index = 0; attr_index < partition_attribute_ids_.size(); ++attr_index) {
+ if (less_unchecked_comparators_[attr_index]->compareTypedValues(lhs[attr_index], rhs[attr_index])) {
+ break;
--- End diff --
Do you mean to use ``continue`` here? The break will cause the control to come out of the for loop and we wouldn't have compared all the attributes, subsequently returning true. Am I missing something?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107975788
--- Diff: query_optimizer/resolver/Resolver.cpp ---
@@ -724,6 +708,25 @@ const S::PartitionSchemeHeader* Resolver::resolvePartitionClause(
THROW_SQL_ERROR_AT(partition_type_string) << "Unrecognized partition type: " << partition_type;
}
+ proto->set_num_partitions(partition_clause->num_partitions()->long_value());
--- End diff --
Do we perform error checks for the following conditions?
1. Number of partitions = 0
2. No attribute specified in the partitioning attributes' list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r108065662
--- Diff: catalog/tests/PartitionScheme_unittest.cpp ---
@@ -611,6 +665,7 @@ TEST(PartitionSchemeTest, CheckRangePartitionSchemeSerialization) {
EXPECT_EQ(blocks_in_part_scheme, blocks_in_part_scheme_from_proto);
}
}
+*/
--- End diff --
Let's not check in commented code. We can either get rid of these tests or try to fix it. I am not sure what these tests are trying to verify. If the functionality being tested is not really critical, we can remove the commented code, create a JIRA issue and move on.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107954118
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -288,22 +310,46 @@ class RangePartitionSchemeHeader : public PartitionSchemeHeader {
* @brief Check if the partition range boundaries are in ascending order.
**/
void checkPartitionRangeBoundaries() {
+ for (const PartitionValues &partition_range_boundary : partition_range_boundaries_) {
+ CHECK_EQ(partition_attribute_ids_.size(), partition_range_boundary.size())
+ << "A partition boundary has different size than that of partition attributes.";
+ }
+
for (partition_id part_id = 1; part_id < partition_range_boundaries_.size(); ++part_id) {
- if (less_unchecked_comparator_->compareTypedValues(
- partition_range_boundaries_[part_id],
- partition_range_boundaries_[part_id - 1])) {
- LOG(FATAL) << "Partition boundaries are not in ascending order.";
+ CHECK(lessThan(partition_range_boundaries_[part_id - 1], partition_range_boundaries_[part_id]))
+ << "Partition boundaries are not in ascending order.";
+ }
+ }
+
+ bool lessThan(const PartitionValues &lhs, const PartitionValues &rhs) const {
+ DCHECK_EQ(partition_attribute_ids_.size(), lhs.size());
+ DCHECK_EQ(partition_attribute_ids_.size(), rhs.size());
+
+ for (std::size_t attr_index = 0; attr_index < partition_attribute_ids_.size(); ++attr_index) {
+ if (less_unchecked_comparators_[attr_index]->compareTypedValues(lhs[attr_index], rhs[attr_index])) {
+ break;
--- End diff --
I think your matching definition is as follows:
(x1, x2, x3, ..., xk) < (y1, y2, y3, ..., yk) if (x1 < x2) || (x2, x3, ..., xk) < (y2, y3, ... yk).
If that's correct, this should be clearly stated somewhere in the code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107981179
--- Diff: catalog/tests/PartitionScheme_unittest.cpp ---
@@ -343,15 +363,43 @@ TEST(PartitionSchemeHeaderTest, VarCharRangePartitionSchemeHeaderTest) {
for (size_t i = 0; i < num_strings; ++i) {
EXPECT_EQ(kExpectedPartitions[i],
partition_scheme_header->getPartitionId(
- TypedValue(kVarChar,
- kSampleStrings[i],
- std::strlen(kSampleStrings[i]) + 1)));
+ { TypedValue(kVarChar, kSampleStrings[i], std::strlen(kSampleStrings[i]) + 1) }));
}
}
+TEST(PartitionSchemeHeaderTest, MultiAttributeRangePartitionSchemeHeaderTest) {
+ vector<PartitionSchemeHeader::PartitionValues> partition_ranges;
+ // Partition boundaries are { 0, 0.00000 }, { 10, 10.0000 }, { 20, 20.00000 }
+ // Last partition can hold upto infinity.
+ // First partition can hold from { -infinity, -infinity } to { 0, -2^(-1074) }.
--- End diff --
Sure, and we are talk about the same thing. In fact, `-2^(-1074)` is the largest negative floating point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107981928
--- Diff: catalog/tests/PartitionScheme_unittest.cpp ---
@@ -611,6 +665,7 @@ TEST(PartitionSchemeTest, CheckRangePartitionSchemeSerialization) {
EXPECT_EQ(blocks_in_part_scheme, blocks_in_part_scheme_from_proto);
}
}
+*/
--- End diff --
Yes, this test triggers [a debug check](https://travis-ci.org/apache/incubator-quickstep/jobs/213492259#L3473) in protobuf in the debug builds only.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #217: QUICKSTEP-73: Multi-attribute PartitionSchem...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/217
Hi @zuyu
Finished one round of review. Please let me know when should I take another look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/217
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107974567
--- Diff: catalog/tests/PartitionScheme_unittest.cpp ---
@@ -611,6 +665,7 @@ TEST(PartitionSchemeTest, CheckRangePartitionSchemeSerialization) {
EXPECT_EQ(blocks_in_part_scheme, blocks_in_part_scheme_from_proto);
}
}
+*/
--- End diff --
Is there some issue with the commented code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107969229
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -48,6 +50,13 @@ class Type;
**/
class PartitionSchemeHeader {
public:
+ // A vector for partitioning catalog attribute .
+ typedef std::vector<attribute_id> PartitionAttributeIds;
+
+ // A boundary for multi-attribute partitions.
+ // PartitionValues.size() should be equal to PartitionAttributeIds.size().
+ typedef std::vector<TypedValue> PartitionValues;
--- End diff --
Sorry, the comment is not that clear. This represents the corresponding values for `PartitionAttributeIds`, and is also used to store the partition range boundaries for a range-based partition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #217: QUICKSTEP-73: Multi-attribute Partiti...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/217#discussion_r107947575
--- Diff: catalog/PartitionSchemeHeader.hpp ---
@@ -48,6 +50,13 @@ class Type;
**/
class PartitionSchemeHeader {
public:
+ // A vector for partitioning catalog attribute .
+ typedef std::vector<attribute_id> PartitionAttributeIds;
+
+ // A boundary for multi-attribute partitions.
+ // PartitionValues.size() should be equal to PartitionAttributeIds.size().
+ typedef std::vector<TypedValue> PartitionValues;
--- End diff --
A clearer and more informative name should be ``PartitionBoundaryValues``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---