Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/24674 )
Change subject: IMPALA-15252: Add Python UDF support ...................................................................... Patch Set 7: (12 comments) > Patch Set 6: > > (12 comments) > > Left couple of comments, mostly nit. Thanks for your reviews! http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/common/status-or.h File be/src/common/status-or.h: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/common/status-or.h@53 PS6, Line 53: T& value() & { : DCHECK(ok_); : return result_.value_; : } : T&& value() && { : DCHECK(ok_); : return result_.value_; : } : const T& value() const& { > To make sure value() is not called with ok_=false, and error() not called w Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h File be/src/exprs/python-udf-call.h: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@28 PS6, Line 28: UDF > nit: UDF Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@32 PS6, Line 32: support > nit: support Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@34 PS6, Line 34: required > nit: required Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@39 PS6, Line 39: initialize > nit: initialize Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h@182 PS6, Line 182: } > Checking this could be moved to the start, to not evaluate children if this Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h@304 PS6, Line 304: each e > nit: the Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h@484 PS6, Line 484: batched_ > nit: support Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/slot-ref.cc File be/src/exprs/slot-ref.cc: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/slot-ref.cc@484 PS6, Line 484: row_id > Does this actually help the compiler? Thanks! Removed from this patch. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/slot-ref.cc@487 PS6, Line 487: is_null > Can we make the assumption this is UNLIKELY? Does this not depend entirely Thanks! Removed from this patch. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/runtime/descriptors.cc@751 PS6, Line 751: DCHECK_EQ(raw_val_type, codegen->GetSlotType(type)) : << endl > What is the reason for this change? Is it an optimization? Thanks! Reverted and added tests. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/runtime/fragment-state.h File be/src/runtime/fragment-state.h: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/runtime/fragment-state.h@123 PS6, Line 123: support > nit: supports Thanks! Changed. -- To view, visit http://gerrit.cloudera.org:8080/24674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04207ac53a53381b0dbdb9a7768665fb95aad519 Gerrit-Change-Number: 24674 Gerrit-PatchSet: 7 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Wed, 09 Sep 2026 09:04:16 +0000 Gerrit-HasComments: Yes
