Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8774 )
Change subject: IMPALA-5014: Part 1: Round when casting string to decimal ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/8774/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8774/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-5014: Part 1: Round when casting string to decimal What's left for later parts? http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/exec/hdfs-scanner-ir.cc File be/src/exec/hdfs-scanner-ir.cc: http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/exec/hdfs-scanner-ir.cc@143 PS1, Line 143: Decimal4Value IrStringToDecimal4(const char* s, int len, int type_precision, Should we also be switching to rounding instead of truncation for text scanners? Oh, upon further reading, it doesn't like it doesn't matter since we treat underflow as an error? http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h File be/src/util/string-parser.h: http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@122 PS1, Line 122: static inline void ApplyExponent(int total_digits_count, Should this be private? I don't think it makes sense for anything external to call it. http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@123 PS1, Line 123: int digits_after_dot_count, int type_precision, int8_t exponent, T& value, Our convention is to pass by pointer if it's modified. http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@125 PS1, Line 125: nit unnecessary vertical whitespace http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@127 PS1, Line 127: // Ex: 0.1e3 (which at this point would have precision == 1 and scale == 1), the Thanks for all the examples, this helped a lot when reviewing it. http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@141 PS1, Line 141: if (*precision < *scale) *precision = *scale; Can this be moved into the else branch? If *scale is zero, this shouldn't be possible, right? http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@204 PS1, Line 204: & Remove the reference here? We want value semantics anyway. http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@219 PS1, Line 219: DCHECK DCHECK_GE? http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@268 PS1, Line 268: maximumum maximum http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@620 PS1, Line 620: & Related to my other comment, passing a char value by reference is weird since the pointer is bigger than the value. -- To view, visit http://gerrit.cloudera.org:8080/8774 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd8b92727fb384e6ff2d145e4aab7ae5d27db26d Gerrit-Change-Number: 8774 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 12 Dec 2017 00:43:39 +0000 Gerrit-HasComments: Yes
