Michael Ho has posted comments on this change. Change subject: IMPALA-1619: Support 64-bit allocations. ......................................................................
Patch Set 1: (7 comments) 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. Yes. We probably need to replace the 1GB allocation limit documentation (if we have any) with this one. 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 Done. http://gerrit.cloudera.org:8080/#/c/2781/1/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 645: *ss << boundary_row_.buffer(); > Is this safe? buffer() isn't null-terminated is it? Good point. Fixed. 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'. I tried rephrasing it. See if it's easier to understand. 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 buf Looking at ScannerContext::Stream::GetBytesInternal(), it appears that the buffer returned before may be retained if the buffer contains variable length data such as string. However, we should also set buffer_ to NULL so the next Append() will not free the buffer. Good catch. 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. Done 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 exhaus It's actually run in exhaustive test run only now. See below. It doesn't actually that long, may be 1~2 min or so end to end on my system. -- 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: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
