Tim Armstrong has posted comments on this change. Change subject: IMPALA-2789: More compact mem layout with null bits at the end. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/4673/2/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 303: // TODO: Before committing this patch, modify the other example IR functions. Still relevant? http://gerrit.cloudera.org:8080/#/c/4673/2/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: Line 148: uint8_t* ptr = reinterpret_cast<uint8_t*>(tuple); I feel like this code is being overly clever with the pointer arithmetic. We should probably just use slot_desc->tuple_offset(). Line 214: uint8_t* ptr = reinterpret_cast<uint8_t*>(tuple); Same here http://gerrit.cloudera.org:8080/#/c/4673/2/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: PS2, Line 603: CreateGEP We should actually use CreateInBoundsGEP() here (code elsewhere doesn't do that, but it should). CreateGEP() prevent LLVM from assuming much about the resulting pointer. I.e. can't assume that it is non-NULL or points to the same memory allocation. Line 661: // necessary because the fields are already aligned, so LLVM should not Is this true? The commit message says that there's no padding, which implies that the fields can't always be aligned. http://gerrit.cloudera.org:8080/#/c/4673/2/be/src/testutil/desc-tbl-builder.cc File be/src/testutil/desc-tbl-builder.cc: Line 112: int null_bit = i % 8; So this allocates NULL bits for non-nullable tuples but doesn't actually set them? This doesn't seem to match the frontend. It would be nice actually if we just got the null bit/byte layout info from the frontend so they logic wasn't duplicated. http://gerrit.cloudera.org:8080/#/c/4673/2/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java File fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java: Line 61: * Slots are placed in descending order by size with trailing bytes to store null flags. Can you mention that non-nullable slots don't get null bits allocated? Line 250: numNullBytes_ = (numNullableSlots + 7) / 8; This doesn't match the logic in the backend, which includes all slots in the calculation. -- To view, visit http://gerrit.cloudera.org:8080/4673 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6510c75d841bddafa6638f1bd2ac6731a7053f6 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
