Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove some useless Result #3561

Merged
merged 5 commits into from
Jun 30, 2022
Merged

refactor: remove some useless Result #3561

merged 5 commits into from
Jun 30, 2022

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Jun 29, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title, include

  • Vis::is_set, Bitmap::iter_from: All their call-cite are unwrap. Actually I wanted to remove from Bitmap::is_set but there are too many usages thus not sure whether it's safe.
  • ArrayBuilder::new/with_meta: Their implementation only returns Ok or panics, and I think this interface should not have error.

There are still too many unnecessary Result, so not included in this PR. Also feel free to challenge me that the Result in the interface makes sense

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

@xxchan xxchan changed the title refactor: remove some useless Result style: remove some useless Result Jun 29, 2022
@xxchan xxchan changed the title style: remove some useless Result refactor: remove some useless Result Jun 29, 2022
@xxchan xxchan requested a review from TennyZhuang June 29, 2022 19:27
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #3561 (2f22286) into main (e39e0c9) will increase coverage by 0.00%.
The diff coverage is 93.14%.

@@           Coverage Diff           @@
##             main    #3561   +/-   ##
=======================================
  Coverage   74.38%   74.38%           
=======================================
  Files         771      771           
  Lines      108700   108701    +1     
=======================================
+ Hits        80857    80861    +4     
+ Misses      27843    27840    -3     
Flag Coverage Δ
rust 74.38% <93.14%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/batch/src/executor/delete.rs 79.68% <ø> (ø)
src/batch/src/executor/hash_agg.rs 89.87% <ø> (ø)
src/batch/src/executor/insert.rs 83.62% <ø> (ø)
src/batch/src/executor/merge_sort_exchange.rs 67.88% <ø> (ø)
src/batch/src/executor/order_by.rs 82.75% <ø> (ø)
src/batch/src/executor/table_function.rs 73.02% <ø> (ø)
src/batch/src/executor/update.rs 82.27% <ø> (ø)
src/batch/src/executor/values.rs 97.67% <ø> (ø)
src/common/src/array/macros.rs 100.00% <ø> (ø)
.../src/executor/aggregation/approx_count_distinct.rs 88.14% <0.00%> (ø)
... and 75 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@xxchan xxchan requested review from skyzh and removed request for skyzh June 29, 2022 19:28
@skyzh
Copy link
Contributor

skyzh commented Jun 30, 2022

Previously we add Result to all functions containing memory allocation, so as to properly control memory usage. But I also find it tedious. So maybe it's a good idea to remove them.

@TennyZhuang
Copy link
Collaborator

In fact, memory-control is very hard, we should reconsider them. Before that, LGTM

@TennyZhuang TennyZhuang added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jun 30, 2022
@TennyZhuang
Copy link
Collaborator

Leaving the huge PR stuck is not a good idea. Currently, I decide to merge it to reduce our effort. Only if we have a well-designed memory control mechanism, we can add them back.

@mergify mergify bot merged commit 9abfc35 into main Jun 30, 2022
@mergify mergify bot deleted the xxchan/no-result branch June 30, 2022 05:21
huangjw806 pushed a commit that referenced this pull request Jul 5, 2022
* remove Result on Vis::is_set, Bitmap::iter_from

* remove Result on ArrayBuilder::new/with_meta

* fix

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue type/refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants