Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9239 )
Change subject: IMPALA-6258: Uninitialized tuple pointers in row batch for empty rows ...................................................................... Patch Set 1: (4 comments) Looks good. Mainly just questions about perf. http://gerrit.cloudera.org:8080/#/c/9239/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9239/1//COMMIT_MSG@32 PS1, Line 32: It would be good to do a microbenchmark to make sure we haven't regressed performance (or if we have, so that we know how much). I was trying to think of the worst possible case. I think it might be count(*) from a large kudu table, since I think that can be served from metadata only and we currently create a bunch of empty row batches. Parquet has a special count(*) optimisation and other file formats have to decode rows to compute the count, so they seem less likely to regress. http://gerrit.cloudera.org:8080/#/c/9239/1/be/src/runtime/tuple.h File be/src/runtime/tuple.h: http://gerrit.cloudera.org:8080/#/c/9239/1/be/src/runtime/tuple.h@225 PS1, Line 225: void SetNull(const NullIndicatorOffset& offset) { I'm not sure about adding DCHECKs to all these functions. They're definitely valid but I'm concerned there might be a real impact on the performance of the debug build (which can be annoying for testing, etc). If we set the pointer to a bogus constant we could potentially catch the same set of bugs via the dereference of invalid memory. http://gerrit.cloudera.org:8080/#/c/9239/1/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: http://gerrit.cloudera.org:8080/#/c/9239/1/be/src/runtime/tuple.cc@49 PS1, Line 49: Tuple* Tuple::Poison() { Can we avoid this function call indirection? MemPool just uses a bogus constant pointer value for this purpose. http://gerrit.cloudera.org:8080/#/c/9239/1/be/src/runtime/tuple.cc@87 PS1, Line 87: if (UNLIKELY(this == Poison())) return Poison(); We can skip this case. MemPool::Allocate() actually already returns a poison value for 0 allocations. -- To view, visit http://gerrit.cloudera.org:8080/9239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I298122aaaa7e62eb5971508e0698e189519755de Gerrit-Change-Number: 9239 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 07 Feb 2018 23:53:16 +0000 Gerrit-HasComments: Yes
