Dan Hecht has posted comments on this change. Change subject: IMPALA-3629: Codegen TransferScratchTuples() in hdfs-parquet-scanner ......................................................................
Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/3774/9/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 165: nit: i think we can do without this blank line to help more code fit on a screen. http://gerrit.cloudera.org:8080/#/c/3774/9/be/src/exec/hdfs-parquet-scanner.h File be/src/exec/hdfs-parquet-scanner.h: PS9, Line 334: process_scratch_batch_fn given that only 2 of 4 scanners follow the same pattern (generate WriteAlignedTuples), it seems confusing that we even pass back this function at all to store in codegend_fn_map_. Why don't we just make each scanner remember whatever state it needs to for codegen? And for seq and text that do have some commonality, they can just pass their codegend write_tuples to InitializeWriteTuplesFn() in order to still share that code. How about you do that cleanup as a follow on patch on top of this one so it can be reviewed independently? http://gerrit.cloudera.org:8080/#/c/3774/9/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 689: AddCodegenExecOption(false, status, format_name); what does this print when we hit the default case above? and in the case we call Codegen() and get back ok(), then fn should be not NULL, correct? So, how about instead we remove the intialization at line 668 and add an assignment at line 684 that says that codegen is not implemented for this format. And then DCHECK(fn != NULL || !status.ook()); and change this if-stmt to only check status? -- 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: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
