Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/22280 )
Change subject: IMPALA-13566: Expose query cancellation status to UDFs ...................................................................... Patch Set 3: (3 comments) 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: nit: remove additional space(4 + 1 spaces) http://gerrit.cloudera.org:8080/#/c/22280/3/be/src/exprs/utility-functions-ir.cc@171 PS3, Line 171: for (int remaining_sleep_duration = milliseconds.val; remaining_sleep_duration > 0; : remaining_sleep_duration -= SLEEP_UNINTERRUPTIBLE_INTERVAL_MS) { : if (ctx->IsQueryCancelled()) { : return BooleanVal(true); : } : SleepForMs(min(remaining_sleep_duration, SLEEP_UNINTERRUPTIBLE_INTERVAL_MS)); : } Maybe a simpler approach would help here. Check cancel first, then break into conditionals based on most probable/fastest branches? Also, instead of using min each time, use divide then a while loop. Something like... if (ctx->IsQueryCancelled()) { return ... } else if(remaining_sleep_duration < SLEEP_UNINTERRUPTIBLE_INTERVAL_MS){ SleepForMS(remaining_sleep_duration) // no need to check cancel } else { int sleep_cnt = remaining_sleep_duration / SLEEP_UNINTERRUPTIBLE_INTERVAL_MS // whole loop will skip if sleep_cnt is <0 while(sleep_cnt-- > 0) { check cacnelled, sleep } // sleep remaining, no need for checking cancel } This way, in the best case, no division, copying or loop operations are performed. And in the worst case, division + less operations within loop would be most efficient? 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@231 PS3, Line 231: : nit: no need for line break -- 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: 3 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Thu, 02 Jan 2025 12:21:04 +0000 Gerrit-HasComments: Yes
