Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12068 )
Change subject: IMPALA-7657: Codegen IsNotEmptyPredicate and ValidTupleIdExpr. ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12068/3/be/src/exprs/valid-tuple-id.h File be/src/exprs/valid-tuple-id.h: http://gerrit.cloudera.org:8080/#/c/12068/3/be/src/exprs/valid-tuple-id.h@51 PS3, Line 51: static IntVal GetIntValStatic(ScalarExprEvaluator*, const TupleRow*); I think that this can be simplified: Instead of having this "static" function defined explicitly in the class, create it during codegen, i.e. using LlvmCodeGen::FnPrototype::GeneratePrototype. You can then use a little IRBuilder to call into the GetIntVal function that you've retrieved with GetFunction with an arg list containing 'this' and the args that you generated for the prototype above. By passing 'this' in this way, I think you'll eliminate the need for GetTupleIdsPtrStatic() I think you can then take the original implementation of GetIntVal(), move the whole thing to the -ir file, and just replace the call to tuple_ids_.size() to your GetNumverTupleIds() while gets replaced in codegen. I may be missing something, though, and sorry if I led you a but astray here by saying that I didn't expect you would need to use IRBuilder. I think these same comments apply to the other class as well, though I didn't go over it in detail. http://gerrit.cloudera.org:8080/#/c/12068/3/be/src/exprs/valid-tuple-id.h@61 PS3, Line 61: DCHECK(false) << "Method should be replaced in codegen."; Would this not be hit if you set DISABLE_CODEGEN=true? -- To view, visit http://gerrit.cloudera.org:8080/12068 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb87b9e3b879c278ce8638d97bcb320a7555a6b3 Gerrit-Change-Number: 12068 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Thu, 13 Dec 2018 21:56:10 +0000 Gerrit-HasComments: Yes
