[Impala-ASF-CR] IMPALA-5551: Fix AggregationNode::Close() when Prepare() fails
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
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
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
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
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
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
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
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
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
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