Taras Bobrovytsky 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? There's still decimal -> timestamp. It's not going to be a large patch, but I think it's better to separate them to make it easier to review. 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 scan Yes, on line 146, the result can't be PARSE_UNDERFLOW. So there is no point in trying to round. 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 Done 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. Done http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@125 PS1, Line 125: > nit unnecessary vertical whitespace Done 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. Many of them were there already. I just moved this code into a separate function to make it easier to reason. 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 b Good catch. Moved. If scale is zero, then precision has to be negative for this to be triggered. This should not happen. 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. Done http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@219 PS1, Line 219: DCHECK > DCHECK_GE? Unfortunately DCHECK_GE does not work with int128_t. Added comment. http://gerrit.cloudera.org:8080/#/c/8774/1/be/src/util/string-parser.h@268 PS1, Line 268: maximumum > maximum Done 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 sin Done. Passing by value now. -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 12 Dec 2017 03:57:43 +0000 Gerrit-HasComments: Yes
