I agree, I was thinking the same thing. The context is a kernel that operates on arrays, the output of which may or may not be used in a filter. I guess the kernel could do the conversion but I figured if we can support it here Filter could do the conversion internally (if it makes sense, maybe it already is?)

I agree, I was thinking the same thing. The context is a kernel that operates on arrays, the output of which may or may not be used in a filter. I guess the kernel could do the conversion but I figured if we can support it here Filter could do the conversion internally (if it makes sense, maybe it already is?)

I think this conversion is typically done in coercion -- like in https://docs.rs/datafusion/latest/datafusion/logical_expr/type_coercion/binary/fn.comparison_coercion.html

(which basically means DataFusion would apply a cast internally to cast Dictionary(*, Boolean) --> Boolean

I think that functions like json_contains shouldn't create a DictionaryArray but rather a BooleanArray directly (so no conversion is needed).

Yeah I agree, that's probably the better way to do things but does it hurt to also add this support? I guess it could be a performance foot gun to allow this, although I don't know how bad it would be. The alternative of not implementing this is the foot gun of: you implement a kernel, write some tests for it but not this specific case and then discovering via a user reporting a bug that DataFusion has this limitation and have to go patch your kernel to protect against this case.

Yeah I agree, that's probably the better way to do things but does it hurt to also add this support? I guess it could be a performance foot gun to allow this, although I don't know how bad it would be.

Yeah, I think the potential issue would be that your code creates these DictionaryArrays of Boolean that is quite a bit slower if you had created BooleanArray directly.

If DataFusion errors, then you know to go try and fix it, but if it doesn't error, then you may never know

That being said, I think this particular change is fine and that we could merge it for the reasons discussed above.

Thanks again @adriangb and @Dandandan

See #12511 which is closely related