Dan Hecht has posted comments on this change. Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types ......................................................................
Patch Set 12: (12 comments) http://gerrit.cloudera.org:8080/#/c/7526/12/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: PS12, Line 193: matching : /// native type > Yes it is, that would be another example of a native type along with Timest I find "slot" to be clearest, but I'm also okay now that the comment clarifies. PS12, Line 195: raw_val_ptr > I thought about this more and I don't think it even makes sense to call Sto Yeah, that makes sense. http://gerrit.cloudera.org:8080/#/c/7526/13/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: PS13, Line 194: e.g. delete PS13, Line 200: ol_ Why not just "StringVal"? PS13, Line 201: l*. should that just say "slot"? PS13, Line 207: a nit PS13, Line 232: what's that? PS13, Line 232: : llvm::Value* EqToNativePtr(llvm::Value* na what does that mean? did some search/replace for "native" get too aggressive? http://gerrit.cloudera.org:8080/#/c/7526/13/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 1689: input_vals, dst, &updated_dst_val)); a one line comment explaining why this is skipped would help http://gerrit.cloudera.org:8080/#/c/7526/12/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: Line 888 > Yeah I think it's potentially confusing. At a minimum we'd probably want to Yeah, I agree to defer that, if at all. http://gerrit.cloudera.org:8080/#/c/7526/12/be/src/udf/udf.h File be/src/udf/udf.h: PS12, Line 81: TYPE_FIXED_BUFFER > It seems better to keep them separate. They're not the same data type conce okay PS12, Line 346: For UDAs that need a complex data structure as the intermediate state, the : /// intermediate type should be string and the UDA can cast the ptr to the structure : /// it is using. : /// : /// Memory Management: For allocations that are not returned to Impala, the UDA should use : /// the FunctionContext::Allocate()/Free() methods. In general, Allocate() is called in : /// Init(), and then Free() must be called in both Serialize() and Finalize(), since : /// either of these functions may be called to clean up the state. For StringVal : /// allocations returned to Impala (e.g. returned by UdaSerialize()), the UDA should : /// allocate the result via StringVal(FunctionContext*, int) ctor or the function : /// StringVal::CopyFrom(FunctionContext*, const uint8_t*, size_t) and Impala will : /// automatically handle freeing it. > It seems confusing to document something that actual UDAs can't use. The cl Yeah, that's true, it shouldn't be documented here. I'm just not sure it's clear enough when reading the backend UDA implementations for readers to trace back and understand things given everything is StringVal without enough documentation somewhere explaining how this all works. i.e. some of the explanation of the commit message seems useful to have in the code comments somewhere. -- To view, visit http://gerrit.cloudera.org:8080/7526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c Gerrit-PatchSet: 12 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
