You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2019/03/08 17:26:12 UTC
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12702
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
When PrettyPrinter prints a time measured in seconds, it multiplies the
time by 1000, and prints that value as milliseconds. Because
PrettyPrinter is implemented using templates, the type of the value
depends on the type of the value parameter. For typical values of
seconds the type is an int32_t. When this is multiplied by 1000 it can
therefore overflow, which triggers a DHECK failure in DEBUG builds.
Fix this by instead multiplying the value parameter by the existing
constant 'THOUSAND' which is declared as a int64_t. This produces a
result which is also a int64_t, and which does not overflow so easily.
TESTING:
Add more test cases to pretty-printer-test.cc including cases that
previously caused overflows. Expand the coverage to include cases
printing NanoSeocnds, MillisSeconds and MicroSeconds. These cases are
not supposed to show that PrettyPrinter always behaves consistently, but
to help maintainers avoid regressions when changing PrettyPrinter.
Ran all end-to-end tests.
Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
---
M be/src/util/pretty-printer-test.cc
M be/src/util/pretty-printer.h
2 files changed, 65 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/12702/1
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3895/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 21:09:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2394/ : 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/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 18:09:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 21:09:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 21:09:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Mar 2019 01:21:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
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/12702 )
Change subject: IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
......................................................................
IMPALA-8288: avoid overflow causing DCHECK in PrettyPrint.
When PrettyPrinter prints a time measured in seconds, it multiplies the
time by 1000, and prints that value as milliseconds. Because
PrettyPrinter is implemented using templates, the type of the value
depends on the type of the value parameter. For typical values of
seconds the type is an int32_t. When this is multiplied by 1000 it can
therefore overflow, which triggers a DHECK failure in DEBUG builds.
Fix this by instead multiplying the value parameter by the existing
constant 'THOUSAND' which is declared as a int64_t. This produces a
result which is also a int64_t, and which does not overflow so easily.
TESTING:
Add more test cases to pretty-printer-test.cc including cases that
previously caused overflows. Expand the coverage to include cases
printing NanoSeocnds, MillisSeconds and MicroSeconds. These cases are
not supposed to show that PrettyPrinter always behaves consistently, but
to help maintainers avoid regressions when changing PrettyPrinter.
Ran all end-to-end tests.
Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Reviewed-on: http://gerrit.cloudera.org:8080/12702
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/pretty-printer-test.cc
M be/src/util/pretty-printer.h
2 files changed, 65 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I00d9769cf34e2ccd796ec1cf88797c8f8250f718
Gerrit-Change-Number: 12702
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>