You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2025/09/23 12:04:00 UTC
[PR] Implement AsRef for Schema and Field [arrow-rs]
findepi opened a new pull request, #8417:
URL: https://github.com/apache/arrow-rs/pull/8417
# Which issue does this PR close?
- closes https://github.com/apache/arrow-rs/issues/5819
# Rationale for this change
This allows writing generic code that accepts `&Field`, `Arc<Field>` or `&Arc<Field>`, or iterators of these. Same for `Schema`.
# What changes are included in this PR?
`impl AsRef<Field> for Field`
`impl AsRef<Schema> for Schema`
as suggested by `AsRef` own docs
# Are these changes tested?
yes
# Are there any user-facing changes?
it does not break API. Downstream crates cannot implement `AsRef<Field> for Field`
Downstream crates cannot workaround these either, per https://github.com/apache/arrow-rs/issues/5819#issuecomment-2142432293
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376849398
##########
arrow-schema/src/field.rs:
##########
@@ -1118,6 +1124,19 @@ mod test {
assert!(f1.cmp(&f3).is_lt());
}
+ #[test]
+ #[expect(clippy::needless_borrows_for_generic_args)] // intentional to exercise various references
+ fn test_field_as_ref() {
+ fn accept_ref(_: impl AsRef<Field>) {}
Review Comment:
Thanks that's a very good point @alamb. That indeed works very well for a function taking a field.
Does it generalize to a function that takes a collection (iter / IntoIterator) over any form of field references?
Example: i have locally-owned `Vec<Field>` and want to run a check over them.
Or I have `Fields` from ` Schema`. I want to be able to write a single function that accepts both, without having to do unnecessary `map`s on the call sites.
i added a commit showing how this works with AsRef, but maybe can be achieved without AsRef too?
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "chmp (via GitHub)" <gi...@apache.org>.
chmp commented on PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#issuecomment-3335058940
@alamb yes this is exactly what I had in mind (at least the impl for Field).
However, I only recently realized, there is no way for me to utilize this impl, if I want to keep compatibility of `serde_arrow` with the currently supported versions of arrow (currently `arrow=37` to `arrow=56`). (It's probably time to drop some of those versions, though).
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#issuecomment-3338385317
> @alamb yes this is exactly what I had in mind (at least the impl for Field).
>
> However, I only recently realized, there is no way for me to utilize this impl, if I want to keep compatibility of `serde_arrow` with the currently supported versions of arrow (currently `arrow=37` to `arrow=56` - at some point, I should probably drop some of those versions, I guess).
So does that mean you suggest we don't proceed with this 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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376856880
##########
arrow-schema/src/field.rs:
##########
@@ -132,6 +132,12 @@ impl Hash for Field {
}
}
+impl AsRef<Field> for Field {
Review Comment:
Added a comment
```rust
// AsRef can be used in a function accepting a field.
// However, this case actually works a bit better when function takes `&Field`
```
i agree that for a field-accepting function, `&Field` is a better type choice.
Let's focus on https://github.com/apache/arrow-rs/pull/8417#discussion_r2376849398. If we can also solve this other problem, we don't need any code 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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376849398
##########
arrow-schema/src/field.rs:
##########
@@ -1118,6 +1124,19 @@ mod test {
assert!(f1.cmp(&f3).is_lt());
}
+ #[test]
+ #[expect(clippy::needless_borrows_for_generic_args)] // intentional to exercise various references
+ fn test_field_as_ref() {
+ fn accept_ref(_: impl AsRef<Field>) {}
Review Comment:
Thanks that's a very good point @alamb. That indeed works very well for a function taking a field.
Does it generalize to a function that takes a collection (iter / IntoIterator) over any form of field references?
Example: i have locally-owned `Vec<Field>` and want to run a check over them.
Or I have `Fields` from ` Schema`. I want to be able to write a single function that accepts both, without having to do unnecessary `map`s on the call sites.
```rust
fn accept_refs(_: impl IntoIterator<Item: AsRef<Field>>) {}
```
i added a commit showing how this works with AsRef, but maybe can be achieved without AsRef too?
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "chmp (via GitHub)" <gi...@apache.org>.
chmp commented on PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#issuecomment-3340225101
@alamb Sorry, should've phrased it differently. I plan to wait for some time before adopting `AsRef<Field>` to ensure the arrow versions supported by my crate cover a decent time range. Overall, I think it makes sense to include, as it allows to write APIs that are easier to use.
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376811940
##########
arrow-schema/src/field.rs:
##########
@@ -132,6 +132,12 @@ impl Hash for Field {
}
}
+impl AsRef<Field> for Field {
Review Comment:
My goes is indeed not to call `my_function(field)` when it doesn't need a field.
However, lack of `AsRef<Field>` prevents `my_function(&field)` from working as well
that's probably because `AsRef` has a blanket impl for `&T`
generally for `AsRef` to work with MyType, one has to implement `AsRef for MyType`. that's apparently how Rust folks designed this mechanism. And that's what AsRef doc says too.
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376768923
##########
arrow-schema/src/field.rs:
##########
@@ -132,6 +132,12 @@ impl Hash for Field {
}
}
+impl AsRef<Field> for Field {
Review Comment:
this change means that the rust compiler with automatically make references from owned Fields which seems strange to me -- among other things it makes it less clear at the callsite that a function doesn't need an owned field
So like
```rust
let field = ...
my_function(field)
```
I would expect that my_function requires ownership of the field. If I saw
```rust
let field = ...
my_function(&field)
```
I would know the function doesn't need ownership
##########
arrow-schema/src/field.rs:
##########
@@ -1118,6 +1124,19 @@ mod test {
assert!(f1.cmp(&f3).is_lt());
}
+ #[test]
+ #[expect(clippy::needless_borrows_for_generic_args)] // intentional to exercise various references
+ fn test_field_as_ref() {
+ fn accept_ref(_: impl AsRef<Field>) {}
Review Comment:
I think a more standard signature would be this:
```suggestion
fn accept_ref(_: &Field) {}
```
And then the compiler would call the various `AsRef` impls as appropriate
However, @tustvold said he thought it was fine on https://github.com/apache/arrow-rs/issues/5819 and I would defer to his rust expertise
--
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] Implement AsRef for Schema and Field [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376848056
##########
arrow-schema/src/field.rs:
##########
@@ -132,6 +132,12 @@ impl Hash for Field {
}
}
+impl AsRef<Field> for Field {
Review Comment:
I think I am confused then (unsurprising!)
The tests have these three examples
If you change the signature to the standard
```rurst
fn accept_ref(_: &Field) {}
```
the only one that doesn't work is the first one
```rust
accept_ref(Arc::new(field())); // <-- the compiler flags this as an error
accept_ref(&Arc::new(field()));
accept_ref(&&Arc::new(field()));
```
Which to me is what I wanted (to have to use `&` to signify the function doesn't need an owned Field` 🤔
--
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