Michael Ho has posted comments on this change. Change subject: IMPALA-2399: Memory limit checks for all scanners. ......................................................................
Patch Set 1: (5 comments) I tested with test_mem_usage_scaling by not skipping avro and text. I cannot check in the change to the test yet as avro sometimes uses more memory than expected so more fine tuning is needed. http://gerrit.cloudera.org:8080/#/c/2568/1/be/src/exec/hdfs-avro-scanner.h File be/src/exec/hdfs-avro-scanner.h: Line 181: /// comments below for ReadAvroChar(). > indicate parse_status_ is set with the error Done Line 236: /// new char buffer. It returns true otherwise. > indicate that parse_status_ is set with the error Done http://gerrit.cloudera.org:8080/#/c/2568/1/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 185: RETURN_IF_ERROR(state_->GetQueryStatus()); > how about leaving a comment until it's renamed: Yes, it's for checking UDF status at this point. Done. http://gerrit.cloudera.org:8080/#/c/2568/1/be/src/exec/text-converter.inline.h File be/src/exec/text-converter.inline.h: Line 71: // In other words, 'copy_string' and 'need_escape' will always be false. > What about the type.IsVarLenStringType() false case? This code is unfortunately convoluted. For this path, IsVarLenStringType() can only return false if type == TYPE_CHAR && len <= MAX_CHAR_INLINE_LENGTH. However, codegen doesn't handle TYPE_CHAR so 'reuse_data' is always true for the codegen case. In other words, only interpretation path will call TryAllocate(). The generated code doesn't emit any call to TryAllocate(). Please see the new DCHECKs I added in hdfs-text-scanner.cc and hdf-sequence-scanner.cc for asserting this assumption.The comments here and there have been beefed up. Once we fix the JIRA to allow TYPE_CHAR in codegen, we should fix the generated code for WriteSlot to call TryAllocate(). http://gerrit.cloudera.org:8080/#/c/2568/1/be/src/runtime/string-buffer.h File be/src/runtime/string-buffer.h: Line 106: $0 > does this work for normal string? I thought this was Substitute() magic. Oops. Fixed. -- To view, visit http://gerrit.cloudera.org:8080/2568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e5a56501967a58513888917db5ce66dec4fb5ce Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
