impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2789: More compact mem layout with null bits at the end.
Date Tue, 11 Oct 2016 20:02:02 GMT
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:

File be/src/exec/

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.
File be/src/runtime/

Line 148:         uint8_t* ptr = reinterpret_cast<uint8_t*>(tuple);
> I feel like this code is being overly clever with the pointer arithmetic. W

Line 214:       uint8_t* ptr = reinterpret_cast<uint8_t*>(tuple);
> Same here
File be/src/runtime/

PS2, Line 603: CreateGEP
> We should actually use CreateInBoundsGEP() here (code elsewhere doesn't do 

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

We only lose the alignment once we store several tuples back-to-back. Accesses to the 2nd
tuple's slots may not be aligned.
File be/src/testutil/

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.
File fe/src/main/java/org/apache/impala/analysis/

Line 61:  * Slots are placed in descending order by size with trailing bytes to store null
> 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 th
Always computed in FE now.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6510c75d841bddafa6638f1bd2ac6731a7053f6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message