You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2018/10/09 17:15:52 UTC

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator 

Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11626


Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................

IMPALA-7677: Fix DCHECK failure in GroupingAggregator

After inserting all of its input into its Aggregators,
StreamingAggregationNode performs some cleanup, such as calling
InputDone() on each Aggregator.

Previously, StreamingAggregationNode only checked that all of the
child's batches had been fetched before doing this cleanup, which
causes problems if the final child batch isn't processed fully in a
single GetNext() call. In this case, multiple calls to InputDone()
lead to a DCHECK failure.

The solution is to only perform the cleanup once the final child batch
has been fully processed.

Testing:
- Added an e2e test with a query that hits this condition.

Change-Id: I851007a60472d0e53081c076c863c866c516677c
---
M be/src/exec/streaming-aggregation-node.cc
M testdata/workloads/functional-query/queries/QueryTest/multiple-distinct-aggs.test
2 files changed, 16 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3342/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Oct 2018 17:04:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has removed Philip Zeyliger from this change.  ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Removed reviewer Philip Zeyliger.
-- 
To view, visit http://gerrit.cloudera.org:8080/11626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 3:

gvo failed due to IMPALA-7746


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Oct 2018 21:06:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/991/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 17:50:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Oct 2018 16:45:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1019/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Oct 2018 23:09:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3342/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Oct 2018 20:49:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3346/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Oct 2018 21:06:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................

IMPALA-7677: Fix DCHECK failure in GroupingAggregator

After inserting all of its input into its Aggregators,
StreamingAggregationNode performs some cleanup, such as calling
InputDone() on each Aggregator.

Previously, StreamingAggregationNode only checked that all of the
child's batches had been fetched before doing this cleanup, which
causes problems if the final child batch isn't processed fully in a
single GetNext() call. In this case, multiple calls to InputDone()
lead to a DCHECK failure.

The solution is to only perform the cleanup once the final child batch
has been fully processed.

Testing:
- Added an e2e test with a query that hits this condition.

Change-Id: I851007a60472d0e53081c076c863c866c516677c
---
M be/src/exec/streaming-aggregation-node.cc
M testdata/workloads/functional-query/queries/QueryTest/multiple-distinct-aggs.test
2 files changed, 16 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Oct 2018 17:04:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

Not super familiar with the code but the fix makes sense to me. Please feel free to get one more review or I can +2 it too.

http://gerrit.cloudera.org:8080/#/c/11626/2/be/src/exec/streaming-aggregation-node.cc
File be/src/exec/streaming-aggregation-node.cc:

http://gerrit.cloudera.org:8080/#/c/11626/2/be/src/exec/streaming-aggregation-node.cc@176
PS2, Line 176: child_batch_.reset();
Not necessary this change but I wonder if this line is necessary given that we usually do this in Close() ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 07:43:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................

IMPALA-7677: Fix DCHECK failure in GroupingAggregator

After inserting all of its input into its Aggregators,
StreamingAggregationNode performs some cleanup, such as calling
InputDone() on each Aggregator.

Previously, StreamingAggregationNode only checked that all of the
child's batches had been fetched before doing this cleanup, which
causes problems if the final child batch isn't processed fully in a
single GetNext() call. In this case, multiple calls to InputDone()
lead to a DCHECK failure.

The solution is to only perform the cleanup once the final child batch
has been fully processed.

Testing:
- Added an e2e test with a query that hits this condition.

Change-Id: I851007a60472d0e53081c076c863c866c516677c
Reviewed-on: http://gerrit.cloudera.org:8080/11626
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/streaming-aggregation-node.cc
M testdata/workloads/functional-query/queries/QueryTest/multiple-distinct-aggs.test
2 files changed, 16 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Oct 2018 01:03:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 )

Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c
Gerrit-Change-Number: 11626
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Oct 2018 21:06:52 +0000
Gerrit-HasComments: No