Dan Hecht has posted comments on this change. Change subject: IMPALA-2399: Memory limit checks for all scanners. ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/2568/2/be/src/exec/hdfs-avro-scanner.h File be/src/exec/hdfs-avro-scanner.h: Line 205: NULL if blocks : /// should be inserted at the end of fn. though the code can still handle it, should we delete this statement? it looks like we always have a block now. http://gerrit.cloudera.org:8080/#/c/2568/2/be/src/exec/hdfs-sequence-scanner.cc File be/src/exec/hdfs-sequence-scanner.cc: Line 256: Codegen will succeed only if there is no TYPE_CHAR : // slots this doesn't explain the escape_char check though. How about mentioning HdfsScanner::InitializeWriteTuplesFn() here. Also, how about clarifying the comment in HdfsScanner::InitializeWriteTuplesFn() to explain why we can't copy the data (I guess that's because we don't call TryAllocate()?) http://gerrit.cloudera.org:8080/#/c/2568/2/be/src/exec/text-converter.inline.h File be/src/exec/text-converter.inline.h: Line 72: // Note that codegen doesn't yet handle TYPE_CHAR. Thanks, the other comments cleared this up a bit but I still find this one confusing. How about something like: The corresponding codegen version of this function (generated by x) does not include this path. 'reuse_data' is always true in the codegen version because of the check in HdfsScanner::InitializeWriteTuplesFn(). Or whatever the correct reason is. -- 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: 2 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
