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

fix(expr, executor): Remove RowExpression: use eval_row and unwrap_or(false) on Datum, not panic on Null/None #3587

Merged
merged 10 commits into from
Jul 6, 2022

Conversation

jon-chuang
Copy link
Contributor

@jon-chuang jon-chuang commented Jul 1, 2022

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

What's changed and what's your intention?

RowExpression: use eval_row and unwrap_or(false) on Datum, not panic on Null/None

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)

@jon-chuang jon-chuang requested a review from yuhao-su July 1, 2022 01:03
@github-actions github-actions bot added the type/fix Bug fix label Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #3587 (d8f8b29) into main (8757d90) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##             main    #3587   +/-   ##
=======================================
  Coverage   74.30%   74.31%           
=======================================
  Files         789      789           
  Lines      111361   111343   -18     
=======================================
- Hits        82746    82739    -7     
+ Misses      28615    28604   -11     
Flag Coverage Δ
rust 74.31% <88.88%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/expr/src/expr/mod.rs 53.48% <ø> (-2.07%) ⬇️
src/stream/src/from_proto/hash_join.rs 0.00% <0.00%> (ø)
src/stream/src/executor/hash_join.rs 97.06% <100.00%> (+0.20%) ⬆️
src/common/src/types/ordered_float.rs 24.70% <0.00%> (-0.20%) ⬇️
src/storage/src/hummock/local_version_manager.rs 81.49% <0.00%> (+0.11%) ⬆️
src/meta/src/barrier/mod.rs 81.74% <0.00%> (+0.38%) ⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs 41.50% <0.00%> (+0.94%) ⬆️

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

@BugenZhao BugenZhao requested a review from TennyZhuang July 1, 2022 02:30
@TennyZhuang
Copy link
Contributor

In fact I don’t know where RowExpression is used.

@yuhao-su
Copy link
Contributor

yuhao-su commented Jul 1, 2022

We can remove the RowExpression in HashJoinExecutor and use Expression instead since we have eval_row now

@TennyZhuang
Copy link
Contributor

We can remove the RowExpression in HashJoinExecutor and use Expression instead since we have eval_row now

Can you remove it?

@jon-chuang
Copy link
Contributor Author

Hmm, ok. I'll do it in this PR.

@jon-chuang jon-chuang changed the title fix(expr, executor): RowExpression: use eval_row and unwrap_or(false) on Datum, not panic on Null/None fix(expr, executor): Remove RowExpression: use eval_row and unwrap_or(false) on Datum, not panic on Null/None Jul 6, 2022
@yuhao-su
Copy link
Contributor

yuhao-su commented Jul 6, 2022

LGTM!

@yuhao-su yuhao-su merged commit 3850d5e into main Jul 6, 2022
@yuhao-su yuhao-su deleted the jon/row_eval branch July 6, 2022 03:44
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
…ap_or(false)` on `Datum`, not panic on Null/None (risingwavelabs#3587)

* eval_row and unwrap_or(false), not panic

* fix

* fmt

* fmt

* fmt

* remove `RowExpression`

* fmt

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants