You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Pranay Singh (Code Review)" <ge...@cloudera.org> on 2017/08/18 20:32:22 UTC
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Pranay Singh has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7725
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
Fix: Current code uses reinterpret_cast for printing
TCounterType::DOUBLE_VALUE, which is unsafe. It also uses a
fixed precision, 2 for printing which may not be desirable.
This change removes the usage of reinterpret_cast and provides
an option to specify precision as an argument type to
Print TCounterType::DOUBLE_VALUE.
Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
---
M be/src/util/pretty-printer-test.cc
M be/src/util/pretty-printer.h
2 files changed, 9 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/7725/1
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has abandoned this change. ( http://gerrit.cloudera.org:8080/7725 )
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
Abandoned
After talking to Tim and Joe, it appears that TUnit::DOUBLE_VALUE is needed and cannot be removed. The use of TUnit::DOUBLE_VALUE can be seen in the function RuntimeProfile::PrintChildCounters(), which calls PrettyPrinter::Print() for printing different TUnit types.
We can't differentiate between the counters that need to be displayed with a decimal precision of FIXED_PRECISION from the other TUnit::UNIT types, if we remove TUnit::DOUBLE_VALUE . So we do need TUnit::DOUBLE_VALUE to have counters that need to be displayed as a floating point values with FIXED_PRECISION.
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-Change-Number: 7725
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change.
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7725/2/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:
PS2, Line 142: case TUnit::DOUBLE_VALUE: {
: double output = static_cast<double>(value);
: ss << std::setprecision(precision) << output << " ";
I think TUnit::DOUBLE_VALUE is so similar to TUnit::UNIT that we should be looking to remove uses of DOUBLE_VALUE and allow UNIT to specify a precision.
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has posted comments on this change.
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7725/2/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:
PS2, Line 142: case TUnit::DOUBLE_VALUE: {
: double output = static_cast<double>(value);
: ss << std::setprecision(precision) << output << " ";
> I thought of removing TUnit::DOUBLE_VALUE from the code and replacing it wi
I see that below values displayed in Runtime profile marked as --> are of type TUnit::DOUBLE changing over to TUnit::UNIT will affect their display.
HDFS_SCAN_NODE (id=0):(Total: 179.334ms, non-child: 179.334ms, % non-child: 100.00%)
- AverageHdfsReadThreadConcurrency: 0.00 -->
- AverageScannerThreadConcurrency: 0.00 --->
Hdfs split stats (<volume id>:<# splits>/<split lengths>): 0:24/478.45 KB
- AverageThreadTokens: 0.00 -->
- BloomFilterBytes: 0
File Formats: TEXT/NONE:24
- AverageHdfsReadThreadConcurrency: 0.00 -->
- AverageScannerThreadConcurrency: 0.00 --> TUnit::DOUBLE
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has posted comments on this change.
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7725/2/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:
PS2, Line 142: case TUnit::DOUBLE_VALUE: {
: double output = static_cast<double>(value);
: ss << std::setprecision(precision) << output << " ";
> I think TUnit::DOUBLE_VALUE is so similar to TUnit::UNIT that we should be
I thought of removing TUnit::DOUBLE_VALUE from the code and replacing it with TUnit::UNIT but was perplexed by two cases;
a) TUnit::UNIT only obeys precision for larger values: It conditionally sets the precision for large values greater than 1000, so how should a caller display a smaller value in floating point representation restricted to two decimal places(or any determined by precision). Say this can be done by setting the precision argument but the second issue is more confounding.
b) Printing different variables of TUnit::UNIT type with different precision: Use of common function like RuntimeProfile::PrintChildCounters() to display variables of same type with different precision makes it hard to use TUnit::UNIT type, it appears that TUnit::DOUBLE is a way to Print some variables with smaller value to be restricted to 2 decimal places (or any precision) and the others to be displayed without any restriction.
Hence I thought that TUnit::DOUBLE_VALUE cannot be removed, so made changes for it.
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1649 Pass precision to Print TCounterType::DOUBLE VALUE
Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has uploaded a new patch set (#2).
Change subject: IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
......................................................................
IMPALA-1649 Pass precision to Print TCounterType::DOUBLE_VALUE
Fix: Current code uses reinterpret_cast for printing
TCounterType::DOUBLE_VALUE, which is unsafe. It also uses a
fixed precision, 2 for printing which may not be desirable.
This change removes the usage of reinterpret_cast and provides
an option to specify precision as an argument type to
Print TCounterType::DOUBLE_VALUE.
Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
---
M be/src/util/pretty-printer-test.cc
M be/src/util/pretty-printer.h
M be/src/util/runtime-profile.cc
3 files changed, 20 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/7725/2
--
To view, visit http://gerrit.cloudera.org:8080/7725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc63d8a59bdf175341097df143798fb1a957d93f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>