Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18526 )
Change subject: IMPALA-10851: Codegen for structs ...................................................................... Patch Set 2: (6 comments) Went through the non-struct related refactors, but struct handling is not clear yet. http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/codegen/codegen-anyval.cc@745 PS2, Line 745: builder_->CreateInsertValue(timestamp_value, GetTimeOfDay(), time_of_day_idxs); here and at many other places: indentation +2 http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/scalar-expr-evaluator.h@290 PS2, Line 290: StoreResult Can you add more comments for this function? http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/scalar-expr-evaluator.h@293 PS2, Line 293: nit: -1 line http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/slot-ref.cc File be/src/exprs/slot-ref.cc: http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/slot-ref.cc@486 PS2, Line 486: /// The generated code can be conceptually divided into the following parts: just a brain dump: I was thinking about generalizing the reading logic - so we could have a struct/class like CodegenValueReader, which would include: - llvm::Values for different types, so both len/ptr/data/time_of_day - llvm::BasicBlock for the null and non-null paths Then sources (like SlotRef here, or CodegenAnyval could implement a CodegenReader(), and the consumer could use it without caring about the source. Even if we don't want to generalize it, such a struct could make this function easier to understand and let 3 and 4 to be seperated. Note that I don't have clear understanding of struct handling yet, so what I described above may not be practical with structs. http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/slot-ref.cc@511 PS2, Line 511: llvm::Value* (*args)[2] I think that it would be clearer to split this to eval_ptr and row_ptr http://gerrit.cloudera.org:8080/#/c/18526/2/be/src/exprs/slot-ref.cc@552 PS2, Line 552: tuple_is_nullable_, slot_is_nullable optional: use the members in CodegenNullChecking instead of passing it as parameter? There could be also a function like isSlotNullable() to make checking this simpler. -- 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: 2 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 23 May 2022 09:01:39 +0000 Gerrit-HasComments: Yes
