Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/22280 )
Change subject: IMPALA-13566: Expose query cancellation status to UDFs ...................................................................... Patch Set 7: (6 comments) Thanks for reviewing! http://gerrit.cloudera.org:8080/#/c/22280/3/be/src/exprs/utility-functions-ir.cc File be/src/exprs/utility-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/22280/3/be/src/exprs/utility-functions-ir.cc@172 PS3, Line 172: n > nit: remove additional space(4 + 1 spaces) Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/22280/3/be/src/exprs/utility-functions-ir.cc@172 PS3, Line 172: FunctionContext* ctx, const IntVa > One concern I have about the current implementation is that it may decrease Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/22280/3/be/src/exprs/utility-functions-ir.cc@171 PS3, Line 171: : BooleanVal UtilityFunctions::Sleep(FunctionContext* ctx, const IntVal& milliseconds) { : if (milliseconds.is_null) return BooleanVal::null(); : for (auto remaining_sleep_duration = chrono::milliseconds(milliseconds.val); : remaining_sleep_duration > chrono::milliseconds(0);) { : if (ctx->IsQueryCancelled()) { : > Thanks for noting that. My intention was also just to keep the sleep functi Thanks! To make sleep() more precise, the new Patch Set checks the actual sleep duration in the loop. Is that OK? http://gerrit.cloudera.org:8080/#/c/22280/3/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22280/3/tests/common/impala_connection.py@618 PS3, Line 618: """Return the raw HS2 result set, which is a list of tuples.""" : return self.__result_tuples : : def __conve > This will ping the coordinator very frequently for the state. Some small sl Thanks! Added. http://gerrit.cloudera.org:8080/#/c/22280/3/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/22280/3/tests/query_test/test_cancellation.py@212 PS3, Line 212: LA-8411): Remove > Is it really needed to run this serially? Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/22280/3/tests/query_test/test_cancellation.py@228 PS3, Line 228: cel > This timeout looks a bit strict to me and I am worried about the test being Thanks! Changed. -- To view, visit http://gerrit.cloudera.org:8080/22280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9430167f7e46bbdf66153abb4645541cd8cf0142 Gerrit-Change-Number: 22280 Gerrit-PatchSet: 7 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Tue, 25 Feb 2025 06:25:55 +0000 Gerrit-HasComments: Yes
