Fix comparison of dictionary arrays #1606
Conversation
| assert_batches_eq!(expected, &actual); | ||
|
|
||
| // comparison with another dictionary column | ||
| let sql = "SELECT d1 FROM test WHERE d1 = d2"; |
There was a problem hiding this comment.
this test fails without the change
alamb
force-pushed
the
alamb/compare_dict_arrays
branch
from
98e574d to
c4db8f3
Compare
| @@ -77,7 +77,7 @@ pub(crate) fn coerce_types( | |||
| } | |||
|
|
|||
| fn comparison_eq_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> { | |||
| if lhs_type == rhs_type { | |||
| if lhs_type == rhs_type && !is_dictionary(lhs_type) { | |||
There was a problem hiding this comment.
the arrow eq kernels don't support DictionaryArray <==> DictionaryArray comparisons yet, so we need to handle this case specially. I will also file a ticket to add native DictionaryArray comparison support.
| @@ -147,6 +147,10 @@ pub fn temporal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<Dat | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn is_dictionary(t: &DataType) -> bool { | |||
There was a problem hiding this comment.
The duplication of code between this module and binary_rule.rs is not good -- I am going to try and consolidate it as a follow on PR cc @liukun4515
There was a problem hiding this comment.
#1607 <-- doesn't remove duplication but it at least consolidates the logic into a single module
| @@ -77,7 +77,7 @@ pub(crate) fn coerce_types( | |||
| } | |||
|
|
|||
| fn comparison_eq_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> { | |||
| if lhs_type == rhs_type { | |||
| if lhs_type == rhs_type && !is_dictionary(lhs_type) { | |||
There was a problem hiding this comment.
Can you add TODO or some comments for this change?
There was a problem hiding this comment.
I will do so -- it is a good point
| @@ -90,7 +90,7 @@ fn comparison_order_coercion( | |||
| lhs_type: &DataType, | |||
| rhs_type: &DataType, | |||
| ) -> Option<DataType> { | |||
| if lhs_type == rhs_type { | |||
| if lhs_type == rhs_type && !is_dictionary(lhs_type) { | |||
| @@ -147,6 +147,10 @@ pub fn temporal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<Dat | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn is_dictionary(t: &DataType) -> bool { | |||
There was a problem hiding this comment.
another point I want to point out is that the arrow-rs has some codes to check data type, and the logic is also in datafusion, for example, https://docs.rs/arrow/7.0.0/arrow/datatypes/enum.DataType.html#method.is_numeric, Maybe we should refactor them and make the codebase clear.
Which issue does this PR close?
Closes #1605
See ticket for more details