You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/09 07:24:09 UTC
[GitHub] [arrow-datafusion] xudong963 opened a new pull request, #2186: make nightly clippy happy
xudong963 opened a new pull request, #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186
- replace `abs` with `unsigned_abs`
- avoid await while holding a non-async-aware MutexGuard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2186: make nightly clippy happy
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186#discussion_r846767435
##########
ballista/rust/scheduler/src/state/persistent_state.rs:
##########
@@ -124,28 +124,33 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>
.get_from_prefix(&get_stage_prefix(&self.namespace))
.await?;
+ let mut tmp_stages: HashMap<StageKey, Arc<dyn ExecutionPlan>> = HashMap::new();
Review Comment:
This makes sense to me (make the snapshot while not holding the lock) but I am not familiar enough with the ballista codebase to know if it would cause problems. Perhaps @mingmwang @yahoNanJing or @thinkharderdev could review it too?
##########
datafusion/physical-expr/src/expressions/lead_lag.rs:
##########
@@ -151,11 +151,11 @@ fn shift_with_default_value(
create_empty_array(value, array.data_type(), array.len())
} else {
let slice_offset = (-offset).clamp(0, value_len) as usize;
- let length = array.len() - offset.abs() as usize;
+ let length = array.len() - offset.unsigned_abs() as usize;
Review Comment:
looking at this code I wonder if we need to check for `offset < 0` -- however, this PR seems like a definite improvement so 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 commented on pull request #2186: make nightly clippy happy
Posted by GitBox <gi...@apache.org>.
xudong963 commented on PR #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186#issuecomment-1094423575
Thanks @alamb @thinkharderdev
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on a diff in pull request #2186: make nightly clippy happy
Posted by GitBox <gi...@apache.org>.
mingmwang commented on code in PR #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186#discussion_r846906225
##########
ballista/rust/scheduler/src/state/persistent_state.rs:
##########
@@ -124,28 +124,33 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>
.get_from_prefix(&get_stage_prefix(&self.namespace))
.await?;
+ let mut tmp_stages: HashMap<StageKey, Arc<dyn ExecutionPlan>> = HashMap::new();
Review Comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] thinkharderdev commented on a diff in pull request #2186: make nightly clippy happy
Posted by GitBox <gi...@apache.org>.
thinkharderdev commented on code in PR #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186#discussion_r846835681
##########
ballista/rust/scheduler/src/state/persistent_state.rs:
##########
@@ -124,28 +124,33 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>
.get_from_prefix(&get_stage_prefix(&self.namespace))
.await?;
+ let mut tmp_stages: HashMap<StageKey, Arc<dyn ExecutionPlan>> = HashMap::new();
Review Comment:
I don't think so. This should only happen when the scheduler initializes and needs to load the persistent state into memory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 merged pull request #2186: make nightly clippy happy
Posted by GitBox <gi...@apache.org>.
xudong963 merged PR #2186:
URL: https://github.com/apache/arrow-datafusion/pull/2186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org