refactor: RegionEngine::handle_request always returns affected rows

[tisonkun]

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This closes #2784.

@waynexia I wonder if we should add a type alias or wrapper for AffectedRows but I keep use usize in this patch for a first step.

Checklist

• I have written the necessary rustdoc comments.
• I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

[tisonkun]

cc @Dysprosium0626 you can take a review or take over this patch as the assignee. But it happens that I have time to take a look at this issue and it's weeks after your assignment.

[WenyXu]

I prefer using words to align the DQL, DML, DDL, and DCL. The definition of execution is too broad.

[tisonkun]

@WenyXu do you mean revert the name to handle_request but retain the return value type refactor?

[tisonkun]

The reason I try to rename is request is actually non-query request but perhaps execution is not optimal also.

[WenyXu]

@WenyXu do you mean revert the name to handle_request but retain the return value type refactor?

Yes, the execution seems not good either. Maybe it's a better time to rename the method when we need to separate handle_request into several methods.

[tisonkun]

@WenyXu Updated.

[codecov]

Codecov Report

Merging #2874 (911a4c1) into develop (b3ffe5c) will decrease coverage by 0.52%.
Report is 5 commits behind head on develop.
The diff coverage is 86.51%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2874      +/-   ##
===========================================
- Coverage    84.86%   84.35%   -0.52%     
===========================================
  Files          745      749       +4     
  Lines       117222   117896     +674     
===========================================
- Hits         99482    99448      -34     
- Misses       17740    18448     +708     

[evenyag]

Shall we add a RegionResponse struct?

struct RegionResponse {
    affected_rows: usize,
}

[Dysprosium0626]

cc @Dysprosium0626 you can take a review or take over this patch as the assignee. But it happens that I have time to take a look at this issue and it's weeks after your assignment.

@tisonkun I am sorry that I was busy on other things last week. I had done it halfway but delayed because I had to take some time to understand the code structure (I'm new here). Anyway, I have the similar idea as your refraction and thank you for taking over my task. I will take some other tasks.

[tisonkun]

@waynexia I wonder if we should add a type alias or wrapper for AffectedRows but I keep use usize in this patch for a first step.

@evenyag I thought of it either. But lean to keep usize until we found the return value as usize causes confusion.

[waynexia]

@waynexia I wonder if we should add a type alias or wrapper for AffectedRows but I keep use usize in this patch for a first step.

@evenyag I thought of it either. But lean to keep usize until we found the return value as usize causes confusion.

Thanks for your efforts.

For the return type, I prefer at least a type alias over plain usize, which makes it easier to distinguish with other usizes

[tisonkun]

OK. I'll push a new commit later :D

[tisonkun]

@WenyXu @evenyag @waynexia updated.

BTW we have three type alias pub type AffectedRows = usize;. Perhaps we can move them to a common base crate as a follow-up.

• src/common/meta/src/datanode_manager.rs
• src/query/src/table_mutation.rs
• src/store-api/src/region_request.rs (added in this patch)

[waynexia]

Almost LGTM, thanks for this @tisonkun

[tisonkun]

@waynexia Updated at 911a4c1