Tim Armstrong has posted comments on this change. Change subject: IMPALA-3629: Codegen TransferScratchTuples() hdfs-parquet-scanner ......................................................................
Patch Set 5: (5 comments) I think we're nearly there, looking good. http://gerrit.cloudera.org:8080/#/c/3774/5/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: PS5, Line 668: s 'status' I think is a little clearer. Line 671: s = HdfsTextScanner::Codegen(this, conjunct_ctxs_, &fn); Thanks for fixing this in the other scanners. PS5, Line 689: fn == NULL The value of fn is undefined in some cases here if the above functions returned a non-OK status. I think this check should be (!status.ok() || fn == NULL): if we change the Codegen() functions to initialise their output arguments to NULL we can skip the codegen_enabled() check. http://gerrit.cloudera.org:8080/#/c/3774/5/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 689: if (!node->runtime_state()->codegen_enabled()) return Status::OK(); I think this and the other functions should initialise their output arguments to NULL so that they always have a defined value in them if we return Status::OK(). I think if you do this you avoid having to check node->runtime_state()->codegen_enabled() in the caller. http://gerrit.cloudera.org:8080/#/c/3774/5/be/src/exec/hdfs-text-scanner.h File be/src/exec/hdfs-text-scanner.h: Line 62: static Status Codegen(HdfsScanNode*, Line wrapping looks weird here. -- To view, visit http://gerrit.cloudera.org:8080/3774 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic327e437c7cd2b3f92cdb11c1e907bfee2d44ee8 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
