Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/10233 )
Change subject: IMPALA-6946: handle negative counts in RLE decoder ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h File be/src/util/rle-encoding.h: http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@326 PS2, Line 326: LIKELY LIKELY seems to be strangely placed now - both conditions should be included in the likely branch, or maybe only the new condition, as current_value_ == value can be often false, for example in dict encoded pages. http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@679 PS2, Line 679: int32_t num_repeats = NextNumRepeats(); DictDecoder<T>::DecodeNextValue also calls NextNumLiterals() and NextNumRepeats(), and expects uint32_t - can you do some cleanup there? http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@681 PS2, Line 681: uint32_t num_repeats_to_set = : std::min<uint32_t>(num_repeats, num_values_to_consume - num_consumed); We could change this to signed too and remove the template parameter from std::min, as both arguments and the result would be signed. http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@694 PS2, Line 694: uint32_t num_literals_to_set = : std::min<uint32_t>(num_literals, num_values_to_consume - num_consumed); Same as in line 681. -- To view, visit http://gerrit.cloudera.org:8080/10233 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If75ef3fb12494209918c100f26407cd93b17addb Gerrit-Change-Number: 10233 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Comment-Date: Wed, 02 May 2018 14:54:06 +0000 Gerrit-HasComments: Yes
