Alex Behm 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? Yes. The tuple struct changed, so I'd assume there are other example IR that need to be corrected. 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. W Done Line 214: uint8_t* ptr = reinterpret_cast<uint8_t*>(tuple); > Same here Done 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 Done 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 implie My understanding is that here we only care about whether LLVM will add padding to force alignment within a single struct (not across structs). Since we order the slots by descending size and only have power-of-two slot sizes I think that all slots should be aligned. Or am I missing something? We only lose the alignment once we store several tuples back-to-back. Accesses to the 2nd tuple's slots may not 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 se This builder is just for BE unit tests and has no way of declaring non-nullable slots. Anyway, I made the change to have the FE compute the mem layouts. Either solution is fine with me, so let me know which version you prefer. 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? Done Line 250: numNullBytes_ = (numNullableSlots + 7) / 8; > This doesn't match the logic in the backend, which includes all slots in th Always computed in FE now. -- 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: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
