Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16059 )
Change subject: IMPALA-9747: More fine-grained codegen for text file scanners ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/16059/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16059/3//COMMIT_MSG@13 PS3, Line 13: improves Can you add some benchmark results? It would be good to see a case where this made things faster. http://gerrit.cloudera.org:8080/#/c/16059/3//COMMIT_MSG@16 PS3, Line 16: About testing: I guess the mixed case of codegenable and non-codegenable columns is already tested somewhere, but it would be good to point to a table like that. http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner-ir.cc File be/src/exec/hdfs-scanner-ir.cc: http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner-ir.cc@115 PS3, Line 115: copy_string Can't we just pass false in WriteSlot? http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.h@594 PS3, Line 594: we typo: with? http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.h@598 PS3, Line 598: int slot_idx, Tuple* tuple, const char* data, int len, MemPool* pool); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.cc@349 PS3, Line 349: slot_desc->type().type != TYPE_CHAR I would prefer to add a function like SupportsCodegenWriteSlot to TextConverter, as it is generally the responsibility of that class to decide whether it supports codegen for a given type. http://gerrit.cloudera.org:8080/#/c/16059/3/be/src/exec/hdfs-scanner.cc@495 PS3, Line 495: this_arg, slot_idx_value, opaque_tuple_arg, data, len, mem_pool_arg}; nit: +2 indentation -- To view, visit http://gerrit.cloudera.org:8080/16059 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id370193af578ecf23ed3c6bfcc65fec448156fa3 Gerrit-Change-Number: 16059 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 10 Jun 2020 22:09:42 +0000 Gerrit-HasComments: Yes
