Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18526 )
Change subject: IMPALA-10851: Codegen for structs ...................................................................... Patch Set 17: (8 comments) http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval-read-write-info.h File be/src/codegen/codegen-anyval-read-write-info.h: http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval-read-write-info.h@52 PS17, Line 52: CondBranchToAsTrue The semantics were not clear to me at first. Maybe names like BranchToIf / BranchToIfNot would be more descriptive? For "true_block"/"false_block" I think that "else_block" would be clearer, but I am not 100% sure here. http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc@827 PS17, Line 827: llvm::Value* fn_ctx = builder->CreateCall(get_func_ctx_fn, : {read_write_info.GetEval(), codegen->GetI32Constant(read_write_info.GetFnCtxIdx())}, : "fn_ctx"); optional: move this logic to CodegenAnyValReadWriteInfo, e.g a getFnCtxCodegend()? while it is not directly related to the class's functionality, it looks convenient as all needed members are there http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc@831 PS17, Line 831: allocalion typo http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc@832 PS17, Line 832: null Shouldn't we return an error if it fails? Or the FunctionContext will set to an error state during the failed allocation? I see that https://github.com/apache/impala/blob/master/be/src/udf/udf.cc#L559 also doesn't return an error, but I think that we should add a comment about the behavior at both places. http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc@852 PS17, Line 852: ColumnType child_type = child_codegen_value_read_write_info.type(); : child_type.children.clear(); : child_type.field_names.clear(); : child_type.field_ids.clear(); : llvm::ConstantStruct* child_type_ir = child_type.ToIR(codegen); optional: move to CodegenAnyValReadWriteInfo as getTypeIR()? http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/codegen/codegen-anyval.cc@890 PS17, Line 890: CodegenAnyVal Is it a good idea to copy this class? I don't think that it should cause problems, but copying the whole class with all its children looks unnecessary. Maybe some smart pointer could be used instead? http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/exec/hash-table.cc@792 PS17, Line 792: CodegenConvertToCanonicalForm Could be moved to CodegenAnyValReadWriteInfo? http://gerrit.cloudera.org:8080/#/c/18526/17/be/src/exec/hash-table.cc@810 PS17, Line 810: CodegenConvertToCanonicalForm This could and the one at line 767 could be moved to CodegenAnyVal? -- To view, visit http://gerrit.cloudera.org:8080/18526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5272c3f095fd9f07877104ee03c8e43d0c4ec0b6 Gerrit-Change-Number: 18526 Gerrit-PatchSet: 17 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 06 Apr 2023 12:55:28 +0000 Gerrit-HasComments: Yes
