fix: broadcasting of unit LHS in string operations

[nameexhaustion]

Would fix #12632.

[ritchie46]

I don't think we should automatically broadcast. That has implicit performance implications. I asserted the invariant in #12738 and ensure that the caller upholds it.

[nameexhaustion]

I don't think it's broadcasting Performance should be ok - I'm using std::iter::repeat and some macro magic to avoid materializing the entire column values :) (and also avoiding Box<dyn> :D).

[ritchie46]

Ah, right. In that case I think it is a nice feature, but I think it should be in a special broadcasting variant of binary_element_apply.

Otherwise we will monomorphize both code variants where in many cases we only need one of the two.

[orlp]

@nameexhaustion Why all the macros?

[nameexhaustion]

Mainly to be able to apply a function to N inputs that are conditionally broadcasted using iter::repeat without heap allocating/dynamic dispatch (I was assuming we wanted to avoid doing that here).

Although given that we currently only need to handle 2 inputs, I suppose it could also be a function instead.

[orlp]

Rather than using an indirect implementation using std::iter::repeat can't you just directly implement the broadcast?

[nameexhaustion]

Hmm - I could also just do ca.new_from_index(0) - it would make things much simpler, but it would materialize an entire array with the same values.

Otherwise I don't know of any better way to do the broadcasting o.o

[orlp]

@nameexhaustion As an example in pseudocode, for binary_elementwise_broadcast(lhs, rhs, op), add an unary_elementwise(ca, op) function with similar semantics. Then if lhs.len() == 1 you call unary_elementwise(rhs, |r| op(lhs[0], r)) and if rhs.len() == 1 you call unary_elementwise(lhs, |l| op(l, rhs[0])).

Note to self: at some point in the near future I want to do a pass over the function names in arity.rs to give them a clearer naming scheme because right now it's a bit all over the place.

[orlp]

Sorry for being so picky 😅 But these functions could get used a lot so we should get them right.

[orlp]

Same here.

[nameexhaustion]

Just to confirm - this would mean requiring unary functions that take Option - i.e. F: FnMut(Option<T::Physical<'a>>) -> Result<Option<K>, E> to always behave as F(None) == None - as otherwise the fast-path would be incorrect.

[nameexhaustion]

Although, we could also cheaply elide this requirement by checking the output of F(input[0]) if input.len() == input.null_count()

[nameexhaustion]

Hmm.. strictly speaking this would also require the function to be Fn instead of FnMut

[orlp]

Unfortunately we can only do a fast-path here if both are full-null... Makes me wonder if this is a good idea after all :( We could still handle that wherever we call them though. Nevertheless, we should do a fast-path here for that case.

[nameexhaustion]

To confirm - this would mean we require F: for<'a> BinaryFnMut<Option<T::Physical<'a>>, Option<U::Physical<'a>>> to always behave as F(None, None) == None

[orlp]

@nameexhaustion Could you resolve the merge conflicts? Then this is ready to go I think.

[nameexhaustion]

@orlp I've rebased