You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by craig-chasseur <gi...@git.apache.org> on 2016/06/12 11:08:11 UTC
[GitHub] incubator-quickstep pull request #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
GitHub user craig-chasseur opened a pull request:
https://github.com/apache/incubator-quickstep/pull/27
QUICKSTEP-18: Allow BasicColumnStoreTupleStorageSubBlock to be unsorted
This makes sorting an optional (instead of required) property of BasicColumnStoreTupleStorageSubBlock.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/craig-chasseur/incubator-quickstep unsorted-column-store
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/27.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 #27
----
commit 7a490fec1717f76b7beecf5bb02e2f0760742ea9
Author: Craig Chasseur <sp...@gmail.com>
Date: 2016-06-12T11:01:17Z
QUICKSTEP-18: Allow BasicColumnStoreTupleStorageSubBlock to be unsorted
----
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/27
Thanks @craig-chasseur! LGTM. Ready to merge, but need you to squash all the commits into one. Can you rebase as per the instructions at: https://cwiki.apache.org/confluence/display/QUICKSTEP/Workflow+For+Committers
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/27
@craig-chasseur Next time when squashing multiple commits, please also edit the commit messages so that the resultant commit message would be better than the combination of all commit messages. Thanks!
```
commit c1476d1e7637a943b00e104cdfd9498e3c7cfe03
Author: Craig Chasseur <sp...@gmail.com>
Date: Sun Jun 12 04:01:17 2016 -0700
QUICKSTEP-18: Allow BasicColumnStoreTupleStorageSubBlock to be unsorted
Review comments applied
Revert build fix for Lexer (separate PR has been opened)
```
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r66725825
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.cpp ---
@@ -183,26 +189,27 @@ bool BasicColumnStoreTupleStorageSubBlock::DescriptionIsValid(
if (description.sub_block_type() != TupleStorageSubBlockDescription::BASIC_COLUMN_STORE) {
return false;
}
- // Make sure a sort_attribute_id is specified.
- if (!description.HasExtension(BasicColumnStoreTupleStorageSubBlockDescription::sort_attribute_id)) {
- return false;
- }
// Make sure relation is not variable-length.
if (relation.isVariableLength()) {
return false;
}
- // Check that the specified sort attribute exists and can be ordered by LessComparison.
- attribute_id sort_attribute_id = description.GetExtension(
- BasicColumnStoreTupleStorageSubBlockDescription::sort_attribute_id);
- if (!relation.hasAttributeWithId(sort_attribute_id)) {
- return false;
- }
- const Type &sort_attribute_type = relation.getAttributeById(sort_attribute_id)->getType();
- if (!ComparisonFactory::GetComparison(ComparisonID::kLess).canCompareTypes(sort_attribute_type,
- sort_attribute_type)) {
- return false;
+ // If a sort attribute is specified, check that it exists and can be ordered
+ // by LessComparison.
+ if (description.HasExtension(
+ BasicColumnStoreTupleStorageSubBlockDescription::sort_attribute_id)) {
+ attribute_id sort_attribute_id = description.GetExtension(
--- End diff --
Could we mark it `const`? 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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r66725964
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.cpp ---
@@ -575,6 +585,13 @@ predicate_cost_t BasicColumnStoreTupleStorageSubBlock::estimatePredicateEvaluati
TupleIdSequence* BasicColumnStoreTupleStorageSubBlock::getMatchesForPredicate(
const ComparisonPredicate &predicate,
const TupleIdSequence *filter) const {
+ if (!sort_specified_) {
+ FATAL_ERROR(
--- End diff --
Could we use `DCHECK(sort_specified_)` from `glog` instead? 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 issue #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/27
@craig-chasseur We are good to merge after addressing one minor issue above, squashing all commits into one, and rebasing on top of the current master branch. 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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
Posted by craig-chasseur <gi...@git.apache.org>.
Github user craig-chasseur commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/27#discussion_r66917331
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.cpp ---
@@ -473,10 +483,10 @@ void BasicColumnStoreTupleStorageSubBlock::setAttributeValueInPlaceTyped(
bool BasicColumnStoreTupleStorageSubBlock::deleteTuple(const tuple_id tuple) {
DEBUG_ASSERT(hasTupleWithID(tuple));
- if (!column_null_bitmaps_.elementIsNull(sort_column_id_)) {
- if (column_null_bitmaps_[sort_column_id_].getBit(tuple)) {
- --(header_->nulls_in_sort_column);
- }
+ if (sort_specified_
+ && !column_null_bitmaps_.elementIsNull(sort_column_id_)
+ && column_null_bitmaps_[sort_column_id_].getBit(tuple)) {
--- End diff --
That's not the style used in this file, nor in the rest of Quickstep generally. See lines 154 and 526 in the same file.
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r66725967
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.cpp ---
@@ -671,6 +688,8 @@ void BasicColumnStoreTupleStorageSubBlock::shiftNullBitmaps(
// total size of tuples contained in this sub-block. It could be done with
// less memory, although the implementation would be more complex.
bool BasicColumnStoreTupleStorageSubBlock::rebuildInternal() {
+ DEBUG_ASSERT(sort_specified_);
--- End diff --
Please use `DCHECK` from `glog`. `DEBUG_ASSERT` has been depreciated.
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/27
@craig-chasseur Please remove all the copyright updates, per discussion in the dev mail list below. Thanks!
> All of your copyright headers need to look exactly like this:
http://www.apache.org/legal/src-headers.html#headers
> In fact, this will be one of the requirements on which your first
ASF release will be predicated. All the other Copyright statements
need to be coalesced and moved into the NOTICE file similar to:
https://github.com/apache/incubator-hawq/blob/master/NOTICE
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/27
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTupleStor...
Posted by craig-chasseur <gi...@git.apache.org>.
Github user craig-chasseur commented on the issue:
https://github.com/apache/incubator-quickstep/pull/27
Completed requested changes.
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r66725906
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.cpp ---
@@ -473,10 +483,10 @@ void BasicColumnStoreTupleStorageSubBlock::setAttributeValueInPlaceTyped(
bool BasicColumnStoreTupleStorageSubBlock::deleteTuple(const tuple_id tuple) {
DEBUG_ASSERT(hasTupleWithID(tuple));
- if (!column_null_bitmaps_.elementIsNull(sort_column_id_)) {
- if (column_null_bitmaps_[sort_column_id_].getBit(tuple)) {
- --(header_->nulls_in_sort_column);
- }
+ if (sort_specified_
+ && !column_null_bitmaps_.elementIsNull(sort_column_id_)
+ && column_null_bitmaps_[sort_column_id_].getBit(tuple)) {
--- End diff --
Code style:
Could we be consistent about where to wrap a line for a conditional expression? I think most of time we use the following style. Thanks!
```
if (aaa &&
bbb &&
cc)
```
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r67010440
--- Diff: storage/BasicColumnStoreTupleStorageSubBlock.hpp ---
@@ -211,11 +213,12 @@ class BasicColumnStoreTupleStorageSubBlock : public TupleStorageSubBlock {
const tuple_id distance);
// Sort all columns according to ascending order of values in the sort
- // column. Returns true if any reordering occured.
+ // column. Returns true if any reordering occured. This should only be called
+ // when 'sort_specified_' is true, otherwise the block is always "built".
bool rebuildInternal();
tuple_id max_tuples_;
- bool sorted_;
+ bool sort_specified_, sorted_;
--- End diff --
Since we set `sort_specified_` in the constructor's initialization list, we could mark `const`.
---
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 #27: QUICKSTEP-18: Allow BasicColumnStoreTu...
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/27#discussion_r66725743
--- Diff: parser/CMakeLists.txt ---
@@ -374,12 +374,15 @@ set_property(SOURCE ${CMAKE_CURRENT_BINARY_DIR}/SqlParser_gen.cpp APPEND PROPERT
include(CheckCXXCompilerFlag)
CHECK_CXX_COMPILER_FLAG("-Wno-deprecated-register" COMPILER_HAS_WNO_DEPRECATED_REGISTER)
if (COMPILER_HAS_WNO_DEPRECATED_REGISTER)
- set_target_properties(quickstep_parser_SqlLexer PROPERTIES COMPILE_FLAGS "-Wno-deprecated-register")
+ set_property(TARGET quickstep_parser_SqlLexer APPEND_STRING PROPERTY COMPILE_FLAGS " -Wno-deprecated-register")
endif()
# GCC will make a warning for unsigned-signed comparisons which are inherent
# in the lexer. For this, we turn off the sign compare.
-set_target_properties(quickstep_parser_SqlLexer PROPERTIES COMPILE_FLAGS "-Wno-sign-compare")
+CHECK_CXX_COMPILER_FLAG("-Wno-sign-compare" COMPILER_HAS_WNO_SIGN_COMPARE)
+if (COMPILER_HAS_WNO_SIGN_COMPARE)
+ set_property(TARGET quickstep_parser_SqlLexer APPEND_STRING PROPERTY COMPILE_FLAGS " -Wno-sign-compare")
+endif()
--- End diff --
@craig-chasseur I think changes in this file are not related to this PR. What do you think of doing so in another separate PR? 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.
---