Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/24674 )
Change subject: IMPALA-15252: Add Python UDF support ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/literal.cc File be/src/exprs/literal.cc: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/literal.cc@474 PS6, Line 474: output[i] = const_cast<void*>(value_.GetRawValue(type_)); Would be cleaner to add non-const accessors or setters than to override the constness like this. http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr-evaluator-ir.cc File be/src/exprs/scalar-expr-evaluator-ir.cc: http://gerrit.cloudera.org:8080/#/c/24674/6/be/src/exprs/scalar-expr-evaluator-ir.cc@38 PS6, Line 38: return output_array[eval->batched_eval_input_->GetRowIdx(row)]; Consider breaking up this function to allow the caller to store the output array pointer. http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/exprs/scalar-expr-evaluator.cc File be/src/exprs/scalar-expr-evaluator.cc: http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/exprs/scalar-expr-evaluator.cc@283 PS7, Line 283: batched_eval_buffers_.resize(root_.batched_eval_output_idx_ + 1, nullptr); Consider using an array of struct where the struct has a buffer and output to avoid passing 2 operands and maintaining parallel arrays. http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/exprs/slot-ref.cc File be/src/exprs/slot-ref.cc: http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/exprs/slot-ref.cc@484 PS7, Line 484: for (int row_idx = 0; row_idx < eval->batched_eval_output_length(); ++row_idx) { Consider use for_each algorithm to facilitate additional parallelism. http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/24674/7/be/src/runtime/row-batch.h@178 PS7, Line 178: return (reinterpret_cast<uint64_t>(row) - reinterpret_cast<uint64_t>(tuple_ptrs_)) Simplify pointer arithmetic or at least use size_t. Probably don't need to cast tuple_ptrs_ at all. -- 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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Wed, 09 Sep 2026 13:14:06 +0000 Gerrit-HasComments: Yes
