impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
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:

(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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message