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 <borokna...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Feb 2018 23:53:16 +0000
Gerrit-HasComments: Yes

Reply via email to