refactor: replace unwrap_or with unwrap_or_else for improved lazy…
#15841
Conversation
github-actions
bot
added
logical-expr
There was a problem hiding this comment.
Thank you @NevroHelios , I triggered the CI, will review after ci passes
|
Thanks for your update @xudong963. I noticed a few checks have failed. I will look into the issues and push the updates shortly. |
…`unwrap_or_else` for better lazy evaluation
|
Hi @xudong963 |
|
This PR seems to have some CI failures Please mark it as ready for review when it is ready for another look |
|
Thank you for this PR @NevroHelios |
|
Hi @alamb, apologies for the delay, I had exams recently. I've run the tests locally and everything looks good now. When you get a chance, could you please re-trigger the CI? Thanks! |
There was a problem hiding this comment.
Thanks @NevroHelios -- this looks like a good set of changes. Thank you -- let's see how the CI goes.
I had one question about one of the changes, but otherwise this looks good to me
benchmarks/src/util/options.rs
Outdated
| pub fn update_config(&self, config: SessionConfig) -> SessionConfig { | ||
| let mut config = config | ||
| .with_target_partitions( | ||
| self.partitions.unwrap_or_else(get_available_parallelism), |
There was a problem hiding this comment.
the original code didn't call get_available_parallelism -- this doesn't look equivalent ot me
I also think the original version of this code was easier to understad -- what is the rationale to change it?
There was a problem hiding this comment.
Thanks for pointing that out! I had originally added get_available_parallelism to fix the clippy and compiler errors. After updating the branch, now it works with the restored default configs.
| @@ -130,8 +130,8 @@ impl Column { | |||
| /// where `"foo.BAR"` would be parsed to a reference to column named `foo.BAR` | |||
| pub fn from_qualified_name(flat_name: impl Into<String>) -> Self { | |||
| let flat_name = flat_name.into(); | |||
| Self::from_idents(parse_identifiers_normalized(&flat_name, false)).unwrap_or( | |||
| Self { | |||
| Self::from_idents(parse_identifiers_normalized(&flat_name, false)).unwrap_or_else( | |||
|
I pushed the updates. Could you please run the ci again? @alamb |
benchmarks/src/util/options.rs
Outdated
| if let Some(sort_spill_reservation_bytes) = self.sort_spill_reservation_bytes { | ||
| config = | ||
| config.with_sort_spill_reservation_bytes(sort_spill_reservation_bytes); |
There was a problem hiding this comment.
I agree it looks like a mistake
There was a problem hiding this comment.
During the copiler errors I updated this config but later I restored most parts but I believe I mistakenly overlooked these lines during the process. Thanks for points that out. I will push the updates shortly.
|
Hi. I pushed the updates. Could you please re run the CI? @alamb |
… evaluation
Which issue does this PR close?
Eliminate the function call in xxx_or (e.g. unwrap_or("".to_string()) #15802
xxx_or (e.g. unwrap_or("".to_string())#15802Rationale for this change
As per the issue explained I updated the function call
unwrap_orwithunwrap_or_elseAre these changes tested?
I build and tested the code locally with
cargo build --workspace -j1andcargo test --workspace -j1@xudong963