Tim Armstrong has posted comments on this change. Change subject: IMPALA-1619: Support 64-bit allocations. ......................................................................
Patch Set 1: (9 comments) I think this is looking pretty good, will take another pass once you've rebased. http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/exec/delimited-text-parser.inline.h File be/src/exec/delimited-text-parser.inline.h: Line 58: // Only support up to 1GB in length for a slot. Truncate if necessary. We probably need some new documentation about this truncation. http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 138: int64_t size = state_->batch_size() * static_cast<int64_t>(tuple_byte_size_); As discussed, will need to fix some of these after rebasing on cdh5-trunk http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 546: if (file_size > byte_buffer_read_size_) { Good catch Line 645: *ss << boundary_row_.buffer(); Is this safe? buffer() isn't null-terminated is it? http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/runtime/free-pool.h File be/src/runtime/free-pool.h: Line 55: 2^62 - 1 - sizeof(FreeListNode) That seems a bit optimistic :) As documentation, makes though. http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/runtime/mem-pool.h File be/src/runtime/mem-pool.h: Line 115: /// ['buf', 'buf' + (size + 7)/8) covers the entire range of a chunk. Would maybe be clearer to say 'rounded up to the nearest 8 bytes'. http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/runtime/string-buffer.h File be/src/runtime/string-buffer.h: Line 67: buffer_size_ = 0; This seems strange. Why would we set buffer_size_ to 0 but not free the buffer? http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/util/decompress-test.cc File be/src/util/decompress-test.cc: Line 125: &compressed)); Indentation. http://gerrit.cloudera.org:8080/#/c/2781/1/tests/query_test/test_compressed_formats.py File tests/query_test/test_compressed_formats.py: Line 151: class TestLargeCompressedFile(ImpalaTestSuite): How long does this test take to run? Should we consider moving it to exhaustive? -- To view, visit http://gerrit.cloudera.org:8080/2781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ed28083d809a86d801a9c063a0aa32c50d32b20 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
