You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by jianqiao <gi...@git.apache.org> on 2017/04/24 15:06:36 UTC
[GitHub] incubator-quickstep pull request #240: Fix the issues with the common subexp...
GitHub user jianqiao opened a pull request:
https://github.com/apache/incubator-quickstep/pull/240
Fix the issues with the common subexpression feature
This PR fixes the issues in #237.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep fix-common-subexpression
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/240.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 #240
----
commit c7333e02fdfcbffe96151f5b5fa2d0c6469d0317
Author: Jianqiao Zhu <ji...@cs.wisc.edu>
Date: 2017-04-24T07:33:39Z
Fix the issues with the common subexpression feature
----
---
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 #240: Fix the issues with the common subexp...
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/240#discussion_r113066280
--- Diff: 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;
--- End diff --
These three methods involve the invoking of `optimizer_context_->nextExprId()` which modifies the context (symbol id counter) of `optimizer_context_`.
---
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 #240: Fix the issues with the common subexp...
Posted by cramja <gi...@git.apache.org>.
Github user cramja commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/240#discussion_r113038771
--- Diff: query_optimizer/rules/ReuseAggregateExpressions.cpp ---
@@ -194,7 +194,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
sum_it == ref_map.end() ? kInvalidRef : sum_it->second.front();
for (const std::size_t idx : avg_it->second) {
- agg_refs[idx].reset(new AggregateReference(sum_ref, count_ref));
+ agg_refs[idx] = std::make_unique<AggregateReference>(sum_ref, count_ref);
--- End diff --
What is the difference between using the assignment and `make_unique` versus `reset` and `new`?
---
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 #240: Fix the issues with the common subexp...
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/240#discussion_r113042124
--- Diff: query_optimizer/rules/ReuseAggregateExpressions.cpp ---
@@ -194,7 +194,7 @@ P::PhysicalPtr ReuseAggregateExpressions::applyToNode(
sum_it == ref_map.end() ? kInvalidRef : sum_it->second.front();
for (const std::size_t idx : avg_it->second) {
- agg_refs[idx].reset(new AggregateReference(sum_ref, count_ref));
+ agg_refs[idx] = std::make_unique<AggregateReference>(sum_ref, count_ref);
--- End diff --
Memory leaks upon system failures when using `new`.
---
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 #240: Fix the issues with the common subexp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/240
---
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 #240: Fix the issues with the common subexp...
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/240#discussion_r113066825
--- Diff: 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;
--- End diff --
Good catch!
---
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 #240: Fix the issues with the common subexp...
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/240#discussion_r113044397
--- Diff: 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;
--- End diff --
All methods below in this file could be marked as `const`:
* `transformExpressions`
* `transformExpression`
* `visitAndTransform`
---
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 #240: Fix the issues with the common subexpression...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/240
@jianqiao We missed the following comments in this PR:
* [Remove unused `moveTypedValue` method in `types/containers/ColumnVector.hpp`](https://github.com/apache/incubator-quickstep/pull/237#discussion_r112827764)
* [Change `hash_cache_` as a `std::uint64_t`, instead of a smart pointer](https://github.com/apache/incubator-quickstep/pull/237#discussion_r112828399)
* [`override` destructor needed for `CommonSubexpression`](https://github.com/apache/incubator-quickstep/pull/237#discussion_r112847577)
---
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.
---