Allow using dictionary arrays as filters

[adriangb]

Fixes #12380

[alamb]

It seems to me that a DictionaryArray with Boolean values is likely to only ever perform worse than a native BooleanArray

The rationale is that each element of a DictionaryArray takes at least 1 byte (for the index into the dictionary when indexed by Int8/UInt8) where each element in a BooleanArray consumes a single bit

So while I think this code is fine and we could merge it, the feature seems suspicious to me 🤔

[adriangb]

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?)

[alamb]

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

[alamb]

I think it makes sense to allow Dictionary as a filter (even though I think subsequent processing will effectively cast it to a BooleanArray)

[Dandandan]

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

[adriangb]

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.

[alamb]

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.

[alamb]

Thanks again @adriangb and @Dandandan

[samuelcolvin]

See #12511 which is closely related