You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/05/28 18:02:11 UTC
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15992
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
IMPALA-9415: Switch result set size calculations from capacity() to size()
The behavior of string's capacity() is implementation specific.
In GCC 7.5.0, the implementation has different behavior compared
to GCC 4.9.2. This is causing a DCHECK to fire in
ClientRequestState::FetchRowsInternal():
// Confirm that this was not an underestimate of the memory required.
DCHECK_GE(before + delta_bytes, after)
What happens on GCC 7.5.0 is that the capacity of the string before the
copy is 29, but after the copy to the result set, the capacity is 30.
The size remains unchanged.
This switches the code to use size(), which is guaranteed to be
consistent across copies. This loses some accuracy, because there is some
string object overhead and excess capacity that no longer counts. However,
this is not code that requires perfect accuracy.
Testing:
- Ran core tests with GCC 4.9.2 and GCC 7.5.0
Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
---
M be/src/service/query-result-set.cc
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/15992/1
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 03:15:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 18:18:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6147/ : 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/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 18:48:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
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/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
IMPALA-9415: Switch result set size calculations from capacity() to size()
The behavior of string's capacity() is implementation specific.
In GCC 7.5.0, the implementation has different behavior compared
to GCC 4.9.2. This is causing a DCHECK to fire in
ClientRequestState::FetchRowsInternal():
// Confirm that this was not an underestimate of the memory required.
DCHECK_GE(before + delta_bytes, after)
What happens on GCC 7.5.0 is that the capacity of the string before the
copy is 29, but after the copy to the result set, the capacity is 30.
The size remains unchanged.
This switches the code to use size(), which is guaranteed to be
consistent across copies. This loses some accuracy, because there is some
string object overhead and excess capacity that no longer counts. However,
this is not code that requires perfect accuracy.
Testing:
- Ran core tests with GCC 4.9.2 and GCC 7.5.0
Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Reviewed-on: http://gerrit.cloudera.org:8080/15992
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/query-result-set.cc
1 file changed, 2 insertions(+), 2 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 20:33:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9415: Switch result set size calculations from capacity() to size()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15992 )
Change subject: IMPALA-9415: Switch result set size calculations from capacity() to size()
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5903/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab260927e14d8951b7c7661f2b5b18a1da39a
Gerrit-Change-Number: 15992
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 20:33:37 +0000
Gerrit-HasComments: No