[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

A recent change moved the initialization of output_tuple_desc_
to the constructor of AggregationNode. This changes the order
in which tuple_pool_ and output_tuple_desc_ are initialized.
The code in AggregationNode::Close() made the assumption that
tuple_pool_ cannot be NULL (although without a DCHECK) if
output_tuple_desc_ is not NULL. This no longer holds in the
new code.

This change makes AggregationNode::Close() checks tuple_pool_
to see if it's NULL before using it.

Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Reviewed-on: http://gerrit.cloudera.org:8080/7254
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/aggregation-node.cc
1 file changed, 3 insertions(+), 1 deletion(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/787/

-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Michael Ho (Code Review) 
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7254

to look at the new patch set (#2).

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..

IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

A recent change moved the initialization of output_tuple_desc_
to the constructor of AggregationNode. This changes the order
in which tuple_pool_ and output_tuple_desc_ are initialized.
The code in AggregationNode::Close() made the assumption that
tuple_pool_ cannot be NULL (although without a DCHECK) if
output_tuple_desc_ is not NULL. This no longer holds in the
new code.

This change makes AggregationNode::Close() checks tuple_pool_
to see if it's NULL before using it.

Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
---
M be/src/exec/aggregation-node.cc
1 file changed, 3 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/7254/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-26 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-22 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7254/1/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

Line 275:   while (!output_iterator_.AtEnd()) {
> It doesn't matter too much so long as we can convince ourselves it's correc
If Prepare() fails, output_iterator_.AtEnd() should be true. I can add a DCHECK 
in Prepare().


-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-21 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7254/1/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

Line 275:   while (!output_iterator_.AtEnd()) {
> Should this be inside the if() statement? Since it references dummy_dst.
It doesn't matter too much so long as we can convince ourselves it's correct 
since this code is near EOL.


-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-21 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7254/1/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

Line 275:   while (!output_iterator_.AtEnd()) {
Should this be inside the if() statement? Since it references dummy_dst.


-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

2017-06-21 Thread Michael Ho (Code Review) 
Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7254

Change subject: IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
..

IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails

A recent change moved the initialization of output_tuple_desc_
to the constructor of AggregationNode. This changes the order
in which tuple_pool_ and output_tuple_desc_ are initialized.
The code in AggregationNode::Close() made the assumption that
tuple_pool_ cannot be NULL (although without a DCHECK) if
output_tuple_desc_ is not NULL. This no longer holds in the
new code.

This change makes AggregationNode::Close() checks tuple_pool_
to see if it's NULL before using it.

Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
---
M be/src/exec/aggregation-node.cc
1 file changed, 2 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/7254/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia0f8cce7cf6e3183d3a5e145c2fcfa50f36c77e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho