Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3629: Codegen TransferScratchTuples() in 
hdfs-parquet-scanner
......................................................................


Patch Set 4:

(6 comments)

I played around with a few different options for exactly what to cross 
compile/constants to replace/etc, and while obviously its workload dependent, I 
think the best option for now is to limit the cross compilation basically just 
to the inner loop of TransferScratchTuples, as I've done here.

http://gerrit.cloudera.org:8080/#/c/3774/3/be/src/exec/hdfs-parquet-scanner-ir.cc
File be/src/exec/hdfs-parquet-scanner-ir.cc:

Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> We'll have to update the license header to the Apache one.
Done


http://gerrit.cloudera.org:8080/#/c/3774/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

PS3, Line 388:   // is non-recoverable, otherwise log the error and continue 
with the next row group.
             :   while (true) {
             :    
> RETURN_IF_ERROR(node->runtime_state()->GetCodegen(&codegen));
Done


PS3, Line 408: set + split_length)) {
             :       // A row group is processed by the scanner whose split 
overl
> codegen->GetIntConstant(sizeof(tuple_byte_size), tuple_byte_size) ?
Done


PS3, Line 520: 
> long line.
Done


http://gerrit.cloudera.org:8080/#/c/3774/3/be/src/exec/hdfs-parquet-scanner.h
File be/src/exec/hdfs-parquet-scanner.h:

Line 446: 
> Maybe document that these are arguments so that they can be replaced by cod
Done


http://gerrit.cloudera.org:8080/#/c/3774/3/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

Line 689:     if (fn == NULL || !s.ok() || !runtime_state()->codegen_enabled()) 
{
> It would be good to rework this so that:
Done


-- 
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: 4
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

Reply via email to