create_array! Drop (Large)Binary special case and support BinaryView

[Tpt]

The three types support From<Vec<&[u8]>> and From<Vec<Option<&[u8]>>>, making the default macro code paths working

Rationale for this change

Simplify code and adds BinaryView support

Are these changes tested?

create_array!(Binary) is already covered in the tests

[Jefffrey]

i'm not sure about this; on main this test compiles fine:

    #[test]
    fn test123() {
        let _ = create_array!(Binary, [&[0], &[1]]);
    }

however with the changes of this PR i get a compile error:

arrow-rs (tpt/create_array_binary)$ cargo test -p arrow-array
   Compiling arrow-array v59.1.0 (/Users/jeffrey/Code/arrow-rs/arrow-array)
error[E0277]: the trait bound `byte_array::GenericByteArray<types::GenericBinaryType<i32>>: From<Vec<&[{integer}; 1]>>` is not satisfied
    --> arrow-array/src/record_batch.rs:130:30
     |
 130 |         std::sync::Arc::new(<$crate::create_array!(@from $ty)>::from(vec![$($values),*]))
     |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unsatisfied trait bound
...
1784 |         let _ = create_array!(Binary, [&[0], &[1]]);
     |                 ----------------------------------- in this macro invocation
     |
help: the trait `From<Vec<&[{integer}; 1]>>` is not implemented for `byte_array::GenericByteArray<types::GenericBinaryType<i32>>`
    --> arrow-array/src/array/byte_array.rs:87:1
     |
  87 | pub struct GenericByteArray<T: ByteArrayType> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = help: the following other types implement trait `From<T>`:
               `byte_array::GenericByteArray<T>` implements `From<arrow_data::ArrayData>`
               `byte_array::GenericByteArray<types::GenericBinaryType<OffsetSize>>` implements `From<Vec<&[u8]>>`
               `byte_array::GenericByteArray<types::GenericBinaryType<OffsetSize>>` implements `From<Vec<Option<&[u8]>>>`
               `byte_array::GenericByteArray<types::GenericBinaryType<OffsetSize>>` implements `From<byte_array::GenericByteArray<types::GenericStringType<OffsetSize>>>`
               `byte_array::GenericByteArray<types::GenericBinaryType<T>>` implements `From<list_array::GenericListArray<T>>`
               `byte_array::GenericByteArray<types::GenericStringType<OffsetSize>>` implements `From<Vec<&str>>`
               `byte_array::GenericByteArray<types::GenericStringType<OffsetSize>>` implements `From<Vec<Option<&str>>>`
               `byte_array::GenericByteArray<types::GenericStringType<OffsetSize>>` implements `From<Vec<Option<String>>>`
             and 3 others
     = note: this error originates in the macro `create_array` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `arrow-array` (lib test) due to 1 previous error

so it seems like a breaking change 🤔

[Tpt]

however with the changes of this PR i get a compile error:

Indeed. This is likely because of a change in type inference: when a function like BinaryArray::from_vec is called (before this MR) Rust will try to do the needed casts to make the call work whereas with BinaryArray::from (after this MR) Rust will try to find a compatible From implementation on the input types without casts and fail.

An option is to add more From implementations to the underlying type. I have just done it in 21196ef

With it the MR has the upside of making create_array!(Binary, [b"foo"]) work but breaks create_array!(Binary, [None]) that needs to be rewritten create_array!(Binary, [None::<&[u8]>]).

What do you think about it?

[Jefffrey]

it's a bit of a niche case indeed; since its technically breaking we'd need to wait for next major release if youre fine with that

(we can split the binaryview addition into a separate PR to merge that one faster)

[Tpt]

Thank you!

it's a bit of a niche case indeed; since its technically breaking we'd need to wait for next major release if youre fine with that

Yes! Of course. Fine to see the MR waiting, I do not have any strong need for it (I got surprised that something I expected to work was not working the way I expected and opened this MR to fix that)

[Jefffrey]

thinking about it some more, im not sure the tradeoff of introducing new From API implementations just for cleaning up some of the macro code is worth it 🤔

unless we have other instances of needing these new From implementations separate from this macro use case

[Tpt]

thinking about it some more, im not sure the tradeoff of introducing new From API implementations just for cleaning up some of the macro code is worth it 🤔

This From API is already implemented on BinaryViewArray and StringArray so it sounds a bit weird to me to not have it supported on Binary. But glad to drop this MR if you feel it's not useful.

[Jefffrey]

thinking about it some more, im not sure the tradeoff of introducing new From API implementations just for cleaning up some of the macro code is worth it 🤔

This From API is already implemented on BinaryViewArray and StringArray so it sounds a bit weird to me to not have it supported on Binary. But glad to drop this MR if you feel it's not useful.

could you elaborate? i was referring to the impl<OffsetSize: OffsetSizeTrait, const C: usize> From<Vec<Option<[u8; C]>>> and co using const generics. these dont seem to exist on the other arrays?

[Tpt]

could you elaborate? i was referring to the impl<OffsetSize: OffsetSizeTrait, const C: usize> From<Vec<Option<[u8; C]>>> and co using const generics. these dont seem to exist on the other arrays?

Sorry, I missunderstood your comment. My usecases are:

• for &[u8; C] it is to make make_array(Binary, [b"foo"]) work. Before any cast, the type of b"foo" is &[u8; 3]. Note that TryFrom<Vec<&[u8; N]>> is already implemented on FixedSizeBinaryArray and I would tend to argue equivalent do not really exist for StringArray or other built-in types.
• for [u8; C] it's mostly for completeness. Glad to drop them if you want

[Jefffrey]

could you elaborate? i was referring to the impl<OffsetSize: OffsetSizeTrait, const C: usize> From<Vec<Option<[u8; C]>>> and co using const generics. these dont seem to exist on the other arrays?

Sorry, I missunderstood your comment. My usecases are:

* for `&[u8; C]` it is to make `make_array(Binary, [b"foo"])` work. Before any cast, the type of `b"foo"` is `&[u8; 3]`. Note that `TryFrom<Vec<&[u8; N]>>` is already implemented on `FixedSizeBinaryArray` and I would tend to argue equivalent do not really exist for `StringArray` or other built-in types.

* for `[u8; C]` it's mostly for completeness. Glad to drop them if you want

    #[test]
    fn test123() {
        let _ = create_array!(Binary, [b"foo"]);
    }

this already compiles fine on main. my point here is that while the cleanup is nice, it forces new From API implementations to ensure it maintains the existing behaviour. if the main driver for this PR is just a cleanup, then i dont think we can justify these new APIs; however if there is a separate need for these new From API implementations then theres more rationale for this change

(we can still add the BinaryView support to the macro, as that is an addition 👍)

[Tpt]

however if there is a separate need for these new From API implementations then theres more rationale for this change

Yes, it make this work:

    #[test]
    fn test123() {
        let _ = create_array!(Binary, [Some(b"foo"), None]);
    }

With the current macro definition, it will expand to BinaryArray::from_vec(vec![Some(b"foo")]) that will fail to compile because from_vec expect a Vec<&u8>. Hence the need of some dynamic dispatch to call either from_vec or from_opt_vec depending on the input type, dispatch that From provides. Note that other datatype (StringArray...) already rely on it to provide Option support (e.g. &str vs Option<&str>).

Sorry for so many back and force, please close if you still disagree and I will open an MR for BinaryView