Balazs Hevele has posted comments on this change. ( http://gerrit.cloudera.org:8080/24674 )
Change subject: IMPALA-15252: Add Python UDF support ...................................................................... Patch Set 6: (12 comments) Left couple of comments, mostly nit. 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() & { return result_.value_; } : T&& value() && { return result_.value_; } : const T& value() const& { return result_.value_; } : const T&& value() const&& { return result_.value_; } : : Status& error() & { return result_.error_; } : Status&& error() && { return result_.error_; } : const Status& error() const& { return result_.error_; } : const Status&& error() const&& { return result_.error_; } To make sure value() is not called with ok_=false, and error() not called with ok_=true, these functions could have DCHECK(ok_) and DCHECK(!ok_) 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: UDFs nit: UDF http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@32 PS6, Line 32: supports nit: support http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@34 PS6, Line 34: requried nit: required http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/python-udf-call.h@39 PS6, Line 39: initialzize nit: initialize 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: return SupportsBatchedEvaluation(); Checking this could be moved to the start, to not evaluate children if this doesn't support batched evaluation: if (!SupportsBatchedEvaluation()) return false; for (const auto& child : children_) { if (!child->TreeSupportsBatchedEvaluation()) return false; } return true; http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h@304 PS6, Line 304: the the nit: the http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr.h@484 PS6, Line 484: supports nit: support 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: LIKELY Does this actually help the compiler? http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/slot-ref.cc@487 PS6, Line 487: UNLIKELY Can we make the assumption this is UNLIKELY? Does this not depend entirely on data/UDF behavior? 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: // TODO: Is this safe? : DCHECK_EQ(raw_val_type->getTypeID(), codegen->GetSlotType(type)->getTypeID()) What is the reason for this change? Is it an optimization? 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: suports nit: supports -- 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: 6 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 03 Sep 2026 11:21:04 +0000 Gerrit-HasComments: Yes
