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
|
|
||
| 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.
| } | ||
| } | ||
|
|
||
| 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
|
|
||
| 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
| rhs_type: &DataType, | ||
| ) -> Option<DataType> { | ||
| if lhs_type == rhs_type { | ||
| if lhs_type == rhs_type && !is_dictionary(lhs_type) { |
| } | ||
| } | ||
|
|
||
| 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.
3 participants
Which issue does this PR close?
Closes #1605
See ticket for more details