Tim Armstrong has posted comments on this change. Change subject: IMPALA-2789: More compact mem layout with null bits at the end. ......................................................................
Patch Set 4: (6 comments) Looking pretty good 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. > Yes. The tuple struct changed, so I'd assume there are other example IR tha IMO we shouldn't feel obliged to keep the sample IR consistent everywhere, just if we're making a change to the function that the comment belongs to. Otherwise the overhead is pretty unreasonable. E.g. for the LLVM 3.8.0 upgrade I would have had to update every comment. http://gerrit.cloudera.org:8080/#/c/4673/4/be/src/exec/row-batch-list-test.cc File be/src/exec/row-batch-list-test.cc: Line 44: scoped_ptr<Frontend> fe; Let's make this a member of RowBatchListTest and lazy-initialise in SetUp(). It's not worth the potential problems with global destructors. http://gerrit.cloudera.org:8080/#/c/4673/4/be/src/runtime/collection-value-builder-test.cc File be/src/runtime/collection-value-builder-test.cc: Line 30: scoped_ptr<Frontend> fe; See my comment in the other test. http://gerrit.cloudera.org:8080/#/c/4673/4/be/src/runtime/row-batch-serialize-test.cc File be/src/runtime/row-batch-serialize-test.cc: Line 44: scoped_ptr<Frontend> fe; Same here http://gerrit.cloudera.org:8080/#/c/4673/4/be/src/testutil/desc-tbl-builder.cc File be/src/testutil/desc-tbl-builder.cc: Line 47: DescriptorTbl* DescriptorTblBuilder::Build() { Thanks, I think this helps with test coverage too. http://gerrit.cloudera.org:8080/#/c/4673/4/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: PS4, Line 755: TTestDescriptorTable TTestDescriptorTableParams? I initially thought this was the descriptor table itself. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
