Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10233 )
Change subject: IMPALA-6946: handle negative counts in RLE decoder ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/10233/3/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/10233/3/be/src/util/dict-encoding.h@238 PS3, Line 238: static constexpr int32_t DICT_DECODER_BUFFER_SIZE = 128; I added some DCHECKS in this file and ran into a linkage issue. The fix was to move it outside of the class (I don't think there's a way to get static linkage for C++ class static members). 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 include Yeah I agree both conditions should be included - 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 NextNumRep Good catch - fixed - I went through the uint32_t references there and fixed those. http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@681 PS2, Line 681: int32_t num_repeats_to_set = : std::min(num_repeats, num_values_to_consume - num_consumed); > We could change this to signed too and remove the template parameter from s Done http://gerrit.cloudera.org:8080/#/c/10233/2/be/src/util/rle-encoding.h@694 PS2, Line 694: int32_t num_literals_to_set = : std::min(num_literals, num_values_to_consume - num_consumed); > Same as in line 681. Done -- 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: 3 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 02 May 2018 17:54:05 +0000 Gerrit-HasComments: Yes
