Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17592 )
Change subject: IMPALA-7635: Reducing HashTable size by packing it's buckets efficiently. ...................................................................... Patch Set 9: (5 comments) Looks really good! Left some minor comments, will do another round tomorrow. http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-ir.cc File be/src/exec/grouping-aggregator-ir.cc: http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-ir.cc@113 PS9, Line 113: false nit: I wonder if instead of true/false we had an enum, then the code could be a bit more self explaining. http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-ir.cc@240 PS9, Line 240: else { : if nit: we could keep the old 'else if' and formatting so it is easier to spot what have changed. http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-ir.cc@245 PS9, Line 245: nit: +2 indent http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-partition.cc File be/src/exec/grouping-aggregator-partition.cc: http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/exec/grouping-aggregator-partition.cc@126 PS9, Line 126: false nit: can we use an enum here as well? http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/util/tagged-ptr.h File be/src/util/tagged-ptr.h: http://gerrit.cloudera.org:8080/#/c/17592/9/be/src/util/tagged-ptr.h@47 PS9, Line 47: #define SET_UNSET_IS_TAG(bit) \ Instead of macros these could be template variables and template functions: template <int bit> static constexpr uintptr_t DATA_MASK = 1ULL << (63 - bit); template <int bit> static constexpr uintptr_t DATA_MASK_INVERSE = ~DATA_MASK<bit>; template <int bit> void SetTagBit() { data_ = (data_ | DATA_MASK<bit>); } ... -- To view, visit http://gerrit.cloudera.org:8080/17592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72912ae9353b0d567a976ca712d2d193e035df9b Gerrit-Change-Number: 17592 Gerrit-PatchSet: 9 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 09 Aug 2021 18:01:34 +0000 Gerrit-HasComments: Yes
