You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by shixuan-fan <gi...@git.apache.org> on 2016/06/27 16:00:12 UTC
[GitHub] incubator-quickstep pull request #40: Minor Change: Added move constructor i...
GitHub user shixuan-fan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/40
Minor Change: Added move constructor in optimizer::logical::Sort
Added move constructor in optimizer::logical::Sort.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep move-semantic-in-logical-sort
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/40.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 #40
----
commit 87c5428a8b295938a89b5b066805a5ea3484c3ef
Author: shixuan-fan <sh...@apache.org>
Date: 2016-06-27T15:50:56Z
Added move semantic in optimizer::logical::Sort
----
---
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 #40: Minor Change: Added move constructor i...
Posted by shixuan-fan <gi...@git.apache.org>.
Github user shixuan-fan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/40#discussion_r68644093
--- Diff: query_optimizer/logical/Sort.hpp ---
@@ -137,6 +150,19 @@ class Sort : public Logical {
addChild(input_);
}
+ Sort(const LogicalPtr &input,
+ const std::vector<expressions::AttributeReferencePtr> &&sort_attributes,
+ const std::vector<bool> &&sort_ascending,
+ const std::vector<bool> &&nulls_first_flags,
+ const int limit)
+ : input_(input),
+ sort_attributes_(std::move(sort_attributes)),
+ sort_ascending_(std::move(sort_ascending)),
+ nulls_first_flags_(std::move(nulls_first_flags)),
+ limit_(limit) {
+ addChild(input_);
+ }
--- End diff --
Thanks! Removed all six `const`s :)
---
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 #40: Minor Change: Use move semantics when ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/40
---
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 #40: Minor Change: Added move constructor i...
Posted by jianqiao <gi...@git.apache.org>.
Github user jianqiao commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/40#discussion_r68642507
--- Diff: query_optimizer/logical/Sort.hpp ---
@@ -137,6 +150,19 @@ class Sort : public Logical {
addChild(input_);
}
+ Sort(const LogicalPtr &input,
+ const std::vector<expressions::AttributeReferencePtr> &&sort_attributes,
+ const std::vector<bool> &&sort_ascending,
+ const std::vector<bool> &&nulls_first_flags,
+ const int limit)
+ : input_(input),
+ sort_attributes_(std::move(sort_attributes)),
+ sort_ascending_(std::move(sort_ascending)),
+ nulls_first_flags_(std::move(nulls_first_flags)),
+ limit_(limit) {
+ addChild(input_);
+ }
--- End diff --
Remove `const` for all rvalue reference (`&&`) arguments, otherwise it is not really doing the move construction.
---
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 #40: Minor Change: Added move constructor in optim...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/40
The title is misleading, please change to `Use move semantics when constructing optimizer::logical::Sort`, since we are not adding a move constructor, but adding a new constructor w/ some rvalue arguments.
Also, we need another separate PR for `WindowPlan`, as discussed in [here](https://github.com/apache/incubator-quickstep/pull/39#discussion_r68328569).
---
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.
---