You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2024/04/03 21:20:12 UTC
[PR] Add Send + Sync traits for Datum [arrow-rs]
viirya opened a new pull request, #5587:
URL: https://github.com/apache/arrow-rs/pull/5587
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
While working on https://github.com/apache/iceberg-rust/pull/295, I faced an issue that a `Datum` cannot be moved into a closure.
```
error[E0277]: `dyn arrow_array::Datum` cannot be sent between threads safely
--> crates/iceberg/src/arrow/reader.rs:451:17
|
449 | PredicateOperator::NotEq => Ok(Box::new(ArrowPredicateFn::new(
| --------------------- required by a bound introduced by this call
450 | self.projection_mask.clone(),
451 | / move |batch| {
452 | | let left = batch.column(term_index);
453 | | neq(left, literal.as_ref())
454 | | },
| |_________________^ `dyn arrow_array::Datum` cannot be sent between threads safely
|
```
Because the function to return `Datum` may return `Result` to propagate error, I cannot put it into the closure.
I'm wondering if we can make `Datum` also `Send` + `Sync`?
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2035721011
Are you able to just add these constraints where necessary? This would avoid making a breaking change
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2038103930
Sorry, I still don't know how can I add `Send` as a constraint to `Scalar` outside this crate. 🤔
I can let the function return `Datum + Send` so `Send` is a constraint of the returned type. But `Scalar` doesn't implement `Send`, how can I make it implement `Send` outside this crate?
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya closed pull request #5587: Add Send + Sync traits for Datum
URL: https://github.com/apache/arrow-rs/pull/5587
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2039944940
I just tried it. Looks like it works.
> Because `Scalar` is not a `Send`, although it is simply a wrapper of `Array`, I guess it cannot be used as `Send` automatically? I think it cannot be but let me try it first. Thanks.
Interesting. So even `Scalar` is not a `Send`, if Rust compiler knows it is a wrapper of `Array`, it will be automatically a `Send`?
```rust
fn get_arrow_datum(datum: &Datum) -> Result<Box<dyn ArrowDatum + Send>> {
match datum.literal() {
PrimitiveLiteral::Boolean(value) => Ok(Box::new(BooleanArray::new_scalar(*value))),
PrimitiveLiteral::Int(value) => Ok(Box::new(Int32Array::new_scalar(*value))),
PrimitiveLiteral::Long(value) => Ok(Box::new(Int64Array::new_scalar(*value))),
PrimitiveLiteral::Float(value) => Ok(Box::new(Float32Array::new_scalar(value.as_f32()))),
PrimitiveLiteral::Double(value) => Ok(Box::new(Float64Array::new_scalar(value.as_f64()))),
l => Err(Error::new(
ErrorKind::DataInvalid,
format!("Unsupported literal type: {:?}", l),
)),
}
}
fn some_func(...) {
let literal = get_arrow_datum(&literal)?;
....
move |batch| {
let left = batch.column(term_index);
lt(left, literal.as_ref())
},
....
}
```
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2035729279
> Are you able to just add these constraints where necessary? This would avoid making a breaking change
Hmm, but the `Datum` is basically `Scalar`. It is a struct not a trait. Can I add `Send` as constraint for it outside this crate?
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2039947410
> `Send` is an auto-trait that will be automatically derived if the contents of the type are themselves `Send`. The same is true for `Sync`.
Ah, got it. I thought it needs to explicitly implement it. Learn it. Thanks.
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2039486444
I'm confused, `Scalar` is just a wrapper around a `T: Array` that implements `Datum`, whether or not `Datum` has additional constraints or not has no bearing on this?
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2039916513
> I'm confused, `Scalar` is just a wrapper around a `T: Array` that implements `Datum`, whether or not `Datum` has additional constraints or not has no bearing on whether `Scalar` is `Send`?
>
> Perhaps you could provide a small code reproducer?
Hmm, do you mean to return `Array` (a `Send`) instead of `Scalar`?
Because `Scalar` is not a `Send`, although it is simply a wrapper of `Array`, I guess it cannot be used as `Send` automatically? I think it cannot be but let me try it first. Thanks.
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2036404582
You can add constraints for marker traits at point of usage
--
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
Re: [PR] Add Send + Sync traits for Datum [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5587:
URL: https://github.com/apache/arrow-rs/pull/5587#issuecomment-2039924444
`Send` is an auto-trait that will be automatically derived if the contents of the type are themselves `Send`. The same is true for `Sync`.
--
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