You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/02 18:14:47 UTC
[GitHub] [arrow-rs] viirya opened a new pull request #1263: wip. Implement DictionaryArray support in eq_dyn
viirya opened a new pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
This is part of #1201.
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold edited a comment on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
tustvold edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038133890
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038119096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038119096
> I was wondering, what does the arrow spec say about dictionary values being null? Probably no one uses that combination, but in theory, a valid key could map to a null value.
https://arrow.apache.org/docs/format/Columnar.html#dictionary-encoded-layout
says explicitly
> Note that a dictionary is permitted to contain duplicate values or nulls:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r802152268
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+{
+ assert_eq!(left.keys().len(), right.keys().len());
Review comment:
Changed to an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold edited a comment on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
tustvold edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038133890
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1028237455
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1263](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d769fd9) into [master](https://codecov.io/gh/apache/arrow-rs/commit/35e16be01e680e9381b2d1393c2e3f8e7acb7b13?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (35e16be) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
+ Coverage 83.04% 83.07% +0.02%
==========================================
Files 180 180
Lines 52424 52502 +78
==========================================
+ Hits 43537 43615 +78
Misses 8887 8887
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `92.12% <100.00%> (+0.41%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.64% <0.00%> (+0.12%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [35e16be...d769fd9](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1028237455
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1263](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9029d46) into [master](https://codecov.io/gh/apache/arrow-rs/commit/aac1844ce707e3744595472b0357c101a91b5749?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aac1844) will **increase** coverage by `0.09%`.
> The diff coverage is `95.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
+ Coverage 82.96% 83.06% +0.09%
==========================================
Files 178 180 +2
Lines 51547 52477 +930
==========================================
+ Hits 42765 43588 +823
- Misses 8782 8889 +107
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.79% <95.00%> (+0.09%)` | :arrow_up: |
| [arrow/src/util/bit\_chunk\_iterator.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvYml0X2NodW5rX2l0ZXJhdG9yLnJz) | `93.47% <0.00%> (-4.33%)` | :arrow_down: |
| [parquet/src/arrow/record\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci5ycw==) | `93.57% <0.00%> (-0.51%)` | :arrow_down: |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `97.58% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet/src/schema/printer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvc2NoZW1hL3ByaW50ZXIucnM=) | `72.32% <0.00%> (-0.16%)` | :arrow_down: |
| [parquet/src/data\_type.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZGF0YV90eXBlLnJz) | `76.49% <0.00%> (-0.12%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.62% <0.00%> (-0.08%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyLnJz) | `78.16% <0.00%> (-0.05%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.73% <0.00%> (-0.04%)` | :arrow_down: |
| ... and [33 more](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [aac1844...9029d46](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r802200374
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,173 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt64Type)
+ }
Review comment:
Seems we don't need implement for other types? `ArrowDictionaryKeyType` is only implemented for Int8Type...UInt64Type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
tustvold commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038133890
From the link @alamb provided
> The null count of such arrays is dictated only by the validity bitmap of its indices, irrespective of any null values in the dictionary.
My interpretation of that is that the nulls on the values array are simply ignored, and only the null mask of the DictionaryArray is considered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r797889057
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,93 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(left: &DictionaryArray<K>, right: &DictionaryArray<K>) -> Result<BooleanArray>
Review comment:
I don't find any `eq` implementation for `DictionaryArray`. Not sure if this is correct approach. cc @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803805872
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4374,4 +4645,192 @@ mod tests {
BooleanArray::from(vec![Some(true), Some(false), Some(true)])
);
}
+
+ fn get_dict_arraydata(
+ keys: Buffer,
+ key_type: DataType,
+ value_data: ArrayData,
+ ) -> ArrayData {
+ let value_type = value_data.data_type().clone();
+ let dict_data_type =
+ DataType::Dictionary(Box::new(key_type), Box::new(value_type));
+ ArrayData::builder(dict_data_type)
+ .len(3)
+ .add_buffer(keys)
+ .add_child_data(value_data)
+ .build()
+ .unwrap()
+ }
+
+ #[test]
+ fn test_eq_dyn_dictionary_i8_array() {
+ let key_type = DataType::Int8;
Review comment:
https://github.com/apache/arrow-rs/issues/1299
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1034033619
@alamb @tustvold This is ready for review. Please take a look again. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1028237455
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1263](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d5ffe59) into [master](https://codecov.io/gh/apache/arrow-rs/commit/35e16be01e680e9381b2d1393c2e3f8e7acb7b13?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (35e16be) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1263 +/- ##
========================================
Coverage 83.04% 83.05%
========================================
Files 180 180
Lines 52424 52844 +420
========================================
+ Hits 43537 43891 +354
- Misses 8887 8953 +66
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `92.29% <100.00%> (+0.58%)` | :arrow_up: |
| [arrow/src/compute/kernels/filter.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9maWx0ZXIucnM=) | `84.77% <0.00%> (-7.73%)` | :arrow_down: |
| [arrow/src/util/bench\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvYmVuY2hfdXRpbC5ycw==) | `95.23% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (ø)` | |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.64% <0.00%> (+0.12%)` | :arrow_up: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.30% <0.00%> (+0.17%)` | :arrow_up: |
| [arrow/src/util/bit\_chunk\_iterator.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvYml0X2NodW5rX2l0ZXJhdG9yLnJz) | `93.83% <0.00%> (+0.35%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [35e16be...d5ffe59](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803756171
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
Review comment:
```suggestion
/// Applies $OP to $LEFT and $RIGHT which are two dictionaries which have (the same) key type $KT
macro_rules! typed_dict_cmp {
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ cmp_dict_utf8::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ cmp_dict_utf8::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Binary, DataType::Binary) => {
+ cmp_dict_binary::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeBinary, DataType::LargeBinary) => {
+ cmp_dict_binary::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampNanosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMicrosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMillisecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Second, _),
+ DataType::Timestamp(TimeUnit::Second, _),
+ ) => {
+ cmp_dict::<$KT, TimestampSecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date32, DataType::Date32) => {
+ cmp_dict::<$KT, Date32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date64, DataType::Date64) => {
+ cmp_dict::<$KT, Date64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::YearMonth),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ cmp_dict::<$KT, IntervalYearMonthType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::DayTime),
+ DataType::Interval(IntervalUnit::DayTime),
+ ) => {
+ cmp_dict::<$KT, IntervalDayTimeType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ ) => {
+ cmp_dict::<$KT, IntervalMonthDayNanoType, _>($LEFT, $RIGHT, $OP)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt64Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Helper function to perform boolean lambda function on values from two dictionary arrays, this
+/// version does not attempt to use SIMD.
Review comment:
```suggestion
/// version does not attempt to use SIMD explicitly (though the compiler may auto vectorize)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ cmp_dict_utf8::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ cmp_dict_utf8::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Binary, DataType::Binary) => {
+ cmp_dict_binary::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeBinary, DataType::LargeBinary) => {
+ cmp_dict_binary::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampNanosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMicrosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMillisecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Second, _),
+ DataType::Timestamp(TimeUnit::Second, _),
+ ) => {
+ cmp_dict::<$KT, TimestampSecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date32, DataType::Date32) => {
+ cmp_dict::<$KT, Date32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date64, DataType::Date64) => {
+ cmp_dict::<$KT, Date64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::YearMonth),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ cmp_dict::<$KT, IntervalYearMonthType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::DayTime),
+ DataType::Interval(IntervalUnit::DayTime),
+ ) => {
+ cmp_dict::<$KT, IntervalDayTimeType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ ) => {
+ cmp_dict::<$KT, IntervalMonthDayNanoType, _>($LEFT, $RIGHT, $OP)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt64Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Helper function to perform boolean lambda function on values from two dictionary arrays, this
+/// version does not attempt to use SIMD.
+macro_rules! compare_dict_op {
+ ($left: expr, $right:expr, $op:expr, $value_ty:ty) => {{
+ if $left.len() != $right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+ let left_values = $left.values().as_any().downcast_ref::<$value_ty>().unwrap();
+ let right_values = $right
+ .values()
+ .as_any()
+ .downcast_ref::<$value_ty>()
+ .unwrap();
+
+ let result = $left
+ .keys()
+ .iter()
+ .zip($right.keys().iter())
+ .map(|(left_key, right_key)| {
+ if let (Some(left_k), Some(right_k)) = (left_key, right_key) {
+ let left_key = left_k.to_usize().expect("Dictionary index not usize");
+ let right_key =
+ right_k.to_usize().expect("Dictionary index not usize");
+ unsafe {
+ let left_value = left_values.value_unchecked(left_key);
+ let right_value = right_values.value_unchecked(right_key);
+ Some($op(left_value, right_value))
+ }
+ } else {
+ None
+ }
+ })
+ .collect();
+
+ Ok(result)
+ }};
+}
+
+/// Perform given operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
Review comment:
```suggestion
/// Returns an error if the two arrays have different value type
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4374,4 +4645,192 @@ mod tests {
BooleanArray::from(vec![Some(true), Some(false), Some(true)])
);
}
+
+ fn get_dict_arraydata(
+ keys: Buffer,
+ key_type: DataType,
+ value_data: ArrayData,
+ ) -> ArrayData {
+ let value_type = value_data.data_type().clone();
+ let dict_data_type =
+ DataType::Dictionary(Box::new(key_type), Box::new(value_type));
+ ArrayData::builder(dict_data_type)
+ .len(3)
+ .add_buffer(keys)
+ .add_child_data(value_data)
+ .build()
+ .unwrap()
+ }
+
+ #[test]
+ fn test_eq_dyn_dictionary_i8_array() {
+ let key_type = DataType::Int8;
Review comment:
Thank you for these tests. It clearly is quite messy to create DictionaryArrays :(
I have some ideas of how to make it less of a mess and will file a follow on ticket / PRs.
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2045,7 +2310,12 @@ macro_rules! typed_compares {
/// assert_eq!(BooleanArray::from(vec![Some(true), None, Some(false)]), result);
/// ```
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, eq_bool, eq, eq_utf8, eq_binary)
+ match left.data_type() {
Review comment:
this is wonderful
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,173 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP_PRIM)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, UInt64Type)
+ }
Review comment:
Yes, I believe that is correct -- the only supported key types are `Int8Type...UInt64Type` (at least in arrow-rs, I am not sure about the arrow spec in general).
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ cmp_dict_utf8::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ cmp_dict_utf8::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Binary, DataType::Binary) => {
+ cmp_dict_binary::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeBinary, DataType::LargeBinary) => {
+ cmp_dict_binary::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampNanosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMicrosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMillisecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Second, _),
+ DataType::Timestamp(TimeUnit::Second, _),
+ ) => {
+ cmp_dict::<$KT, TimestampSecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date32, DataType::Date32) => {
+ cmp_dict::<$KT, Date32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date64, DataType::Date64) => {
+ cmp_dict::<$KT, Date64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::YearMonth),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ cmp_dict::<$KT, IntervalYearMonthType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::DayTime),
+ DataType::Interval(IntervalUnit::DayTime),
+ ) => {
+ cmp_dict::<$KT, IntervalDayTimeType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ ) => {
+ cmp_dict::<$KT, IntervalMonthDayNanoType, _>($LEFT, $RIGHT, $OP)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt64Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Helper function to perform boolean lambda function on values from two dictionary arrays, this
+/// version does not attempt to use SIMD.
+macro_rules! compare_dict_op {
+ ($left: expr, $right:expr, $op:expr, $value_ty:ty) => {{
+ if $left.len() != $right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+ let left_values = $left.values().as_any().downcast_ref::<$value_ty>().unwrap();
+ let right_values = $right
+ .values()
+ .as_any()
+ .downcast_ref::<$value_ty>()
+ .unwrap();
+
+ let result = $left
+ .keys()
+ .iter()
+ .zip($right.keys().iter())
+ .map(|(left_key, right_key)| {
+ if let (Some(left_k), Some(right_k)) = (left_key, right_key) {
+ let left_key = left_k.to_usize().expect("Dictionary index not usize");
+ let right_key =
+ right_k.to_usize().expect("Dictionary index not usize");
+ unsafe {
Review comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r802152137
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
Review comment:
Thanks. Make it generic and rename to `cmp_dict`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1037977838
Looks good. I also find generics to be nicer, mostly because of IDE support, but those would be tricky here since there is no generic trait containing the `value`/`value_unchecked` methods.
I was wondering, what does the arrow spec say about dictionary *values* being null? Probably no one uses that combination, but in theory, a valid key could map to a null value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1028237455
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1263](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (727aba1) into [master](https://codecov.io/gh/apache/arrow-rs/commit/aac1844ce707e3744595472b0357c101a91b5749?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aac1844) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
- Coverage 82.96% 82.95% -0.01%
==========================================
Files 178 180 +2
Lines 51547 52137 +590
==========================================
+ Hits 42765 43250 +485
- Misses 8782 8887 +105
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.89% <100.00%> (+0.19%)` | :arrow_up: |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `97.58% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.52% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyLnJz) | `78.16% <0.00%> (-0.05%)` | :arrow_down: |
| [parquet/src/data\_type.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZGF0YV90eXBlLnJz) | `76.61% <0.00%> (ø)` | |
| [parquet/src/arrow/record\_reader/buffer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci9idWZmZXIucnM=) | `92.42% <0.00%> (ø)` | |
| [arrow/src/util/pretty.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvcHJldHR5LnJz) | `96.66% <0.00%> (ø)` | |
| [parquet/src/arrow/async\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXN5bmNfcmVhZGVyLnJz) | `0.00% <0.00%> (ø)` | |
| [parquet/src/file/footer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZmlsZS9mb290ZXIucnM=) | `95.37% <0.00%> (+0.04%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [aac1844...727aba1](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
tustvold commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038133890
From the link @alamb provided
> The null count of such arrays is dictated only by the validity bitmap of its indices, irrespective of any null values in the dictionary.
My interpretation of that is that the nulls on the values array are simply ignored, and only the null mask of the DictionaryArray is considered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1035199764
@alamb Thanks! Added `Float32` and `Float64` and also changed the comments as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r802152646
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+{
+ assert_eq!(left.keys().len(), right.keys().len());
+
+ let left_values = left
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+ let right_values = right
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+
+ let mut result = vec![];
+
+ for idx in 0..left.keys().len() {
+ unsafe {
Review comment:
Changed to suggested approach. I can work on an iterator for DictionaryArray in a separate PR later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803891260
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4374,4 +4645,192 @@ mod tests {
BooleanArray::from(vec![Some(true), Some(false), Some(true)])
);
}
+
+ fn get_dict_arraydata(
+ keys: Buffer,
+ key_type: DataType,
+ value_data: ArrayData,
+ ) -> ArrayData {
+ let value_type = value_data.data_type().clone();
+ let dict_data_type =
+ DataType::Dictionary(Box::new(key_type), Box::new(value_type));
+ ArrayData::builder(dict_data_type)
+ .len(3)
+ .add_buffer(keys)
+ .add_child_data(value_data)
+ .build()
+ .unwrap()
+ }
+
+ #[test]
+ fn test_eq_dyn_dictionary_i8_array() {
+ let key_type = DataType::Int8;
Review comment:
https://github.com/apache/arrow-rs/pull/1300 as a proposed PR to make it better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803749434
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
Review comment:
Maybe it is also useful to include `Float32` and `Float64` -- but we an also do that as a follow on PR if someone is looking for it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1037977838
Looks good. I also find generics to be nicer, mostly because of IDE support, but those would be tricky here since there is no generic trait containing the `value`/`value_unchecked` methods.
I was wondering, what does the arrow spec say about dictionary *values* being null? Probably no one uses that combination, but in theory, a valid key could map to a null value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1038138028
> i.e. any nulls on the values array are simply ignored, and only the null mask of the indices array (keys in arrow-rs) are considered.
I agree that this is my interpretation as well. Maybe it is worth a PR to clarify the format docs to be super explicit on this point?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1029709894
Thanks for the review comments! I will address them and finish soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r797919798
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
Review comment:
Definitely a bonus feature, but you might want to make this generic in the comparison operation, perhaps look at `simd_compare_op` for inspiration
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
Review comment:
I do wonder if there might be some way to reuse `typed_compares` here somehow, with the notion of the source type somehow encoded in OP_PRIM, etc... but it may be more effort than it is worth
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+{
+ assert_eq!(left.keys().len(), right.keys().len());
Review comment:
Looking at the other implementations this typically returns an error
```
if $left.len() != $right.len() {
return Err(ArrowError::ComputeError(
"Cannot perform comparison operation on arrays of different length"
.to_string(),
));
}
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
Review comment:
It's entirely personal preference, and this file does tend to use macros for this stuff, but _personally_ I find generics easier to interpret, debug and maintain. Just putting the idea out there
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+{
+ assert_eq!(left.keys().len(), right.keys().len());
+
+ let left_values = left
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+ let right_values = right
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+
+ let mut result = vec![];
+
+ for idx in 0..left.keys().len() {
+ unsafe {
+ let left_key = left
+ .keys()
+ .value_unchecked(idx)
+ .to_usize()
+ .expect("Dictionary index not usize");
+ let left_value = left_values.value_unchecked(left_key);
+ let right_key = right
+ .keys()
+ .value_unchecked(idx)
+ .to_usize()
+ .expect("Dictionary index not usize");
+ let right_value = right_values.value_unchecked(right_key);
+
+ result.push(left_value == right_value);
+ }
+ }
+
+ Ok(BooleanArray::from(result))
Review comment:
It is likely to be significantly faster to use `MutableBuffer::from_trusted_len_iter_bool(iter)` here, where `iter` is the body of the for loop. This not only avoids incrementally growing a `Vec`, but also allocating space for `Vec<bool>` and then copying it into a `Buffer` - instead it will construct the packed bitmask directly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r798048841
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
Review comment:
macros are used pretty heavily in this file, so it may be better to follow along with that pattern
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Int8, DataType::Int8) => {
+ $OP_PRIM::<$KT, Int8Type>($LEFT, $RIGHT)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP_PRIM: ident) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+{
+ assert_eq!(left.keys().len(), right.keys().len());
+
+ let left_values = left
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+ let right_values = right
+ .values()
+ .as_any()
+ .downcast_ref::<PrimitiveArray<T>>()
+ .unwrap();
+
+ let mut result = vec![];
+
+ for idx in 0..left.keys().len() {
+ unsafe {
Review comment:
I think you could avoid some amount of this unsafe-ness and Vec manipulation with something like this (untested)
```rust
let result: BooleanArray = left.keys().iter()
.zip(right.keys.iter())
.map(|(left_key, right_key)| {
let left_key = left_key.to_usize().expect("..");
let right_key = right_key.to_usize().expect("..");
...
left_value == right_value
})
.collect()
```
Though given this pattern, perhaps we could (in a separate PR) implement an iterator for DictionaryArray, so this code could devolve into
```rust
let result: BooleanArray = left.iter()
.zip(right.iter())
.map(|(left_value, right_value)| {
left_value == right_value
})
.collect()
```
🤔 then I wonder how many of the existing macros would "just work" 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803913349
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
Review comment:
oops, I forgot `Float32` and `Float64`. Going to add them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803914292
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4374,4 +4645,192 @@ mod tests {
BooleanArray::from(vec![Some(true), Some(false), Some(true)])
);
}
+
+ fn get_dict_arraydata(
+ keys: Buffer,
+ key_type: DataType,
+ value_data: ArrayData,
+ ) -> ArrayData {
+ let value_type = value_data.data_type().clone();
+ let dict_data_type =
+ DataType::Dictionary(Box::new(key_type), Box::new(value_type));
+ ArrayData::builder(dict_data_type)
+ .len(3)
+ .add_buffer(keys)
+ .add_child_data(value_data)
+ .build()
+ .unwrap()
+ }
+
+ #[test]
+ fn test_eq_dyn_dictionary_i8_array() {
+ let key_type = DataType::Int8;
Review comment:
yea, I found it is a bit painful to create dictionary array. thanks for the proposal!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1263: wip. Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#issuecomment-1028237455
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1263](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eff3e10) into [master](https://codecov.io/gh/apache/arrow-rs/commit/aac1844ce707e3744595472b0357c101a91b5749?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aac1844) will **increase** coverage by `0.09%`.
> The diff coverage is `92.68%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
+ Coverage 82.96% 83.05% +0.09%
==========================================
Files 178 180 +2
Lines 51547 52464 +917
==========================================
+ Hits 42765 43573 +808
- Misses 8782 8891 +109
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.72% <92.68%> (+0.02%)` | :arrow_up: |
| [arrow/src/util/bit\_chunk\_iterator.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvYml0X2NodW5rX2l0ZXJhdG9yLnJz) | `93.47% <0.00%> (-4.33%)` | :arrow_down: |
| [parquet/src/arrow/record\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci5ycw==) | `93.57% <0.00%> (-0.51%)` | :arrow_down: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.41%)` | :arrow_down: |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `97.58% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet/src/schema/printer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvc2NoZW1hL3ByaW50ZXIucnM=) | `72.32% <0.00%> (-0.16%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.51% <0.00%> (-0.13%)` | :arrow_down: |
| [parquet/src/data\_type.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZGF0YV90eXBlLnJz) | `76.49% <0.00%> (-0.12%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.62% <0.00%> (-0.08%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyLnJz) | `78.16% <0.00%> (-0.05%)` | :arrow_down: |
| ... and [34 more](https://codecov.io/gh/apache/arrow-rs/pull/1263/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [aac1844...eff3e10](https://codecov.io/gh/apache/arrow-rs/pull/1263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263#discussion_r803923255
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
}};
}
+macro_rules! typed_dict_cmp {
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ match ($LEFT.value_type(), $RIGHT.value_type()) {
+ (DataType::Boolean, DataType::Boolean) => {
+ cmp_dict_bool::<$KT, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int8, DataType::Int8) => {
+ cmp_dict::<$KT, Int8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ cmp_dict::<$KT, Int16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ cmp_dict::<$KT, Int32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ cmp_dict::<$KT, Int64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ cmp_dict::<$KT, UInt8Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ cmp_dict::<$KT, UInt16Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ cmp_dict::<$KT, UInt32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ cmp_dict_utf8::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ cmp_dict_utf8::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Binary, DataType::Binary) => {
+ cmp_dict_binary::<$KT, i32, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::LargeBinary, DataType::LargeBinary) => {
+ cmp_dict_binary::<$KT, i64, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ DataType::Timestamp(TimeUnit::Nanosecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampNanosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMicrosecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ DataType::Timestamp(TimeUnit::Millisecond, _),
+ ) => {
+ cmp_dict::<$KT, TimestampMillisecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Timestamp(TimeUnit::Second, _),
+ DataType::Timestamp(TimeUnit::Second, _),
+ ) => {
+ cmp_dict::<$KT, TimestampSecondType, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date32, DataType::Date32) => {
+ cmp_dict::<$KT, Date32Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (DataType::Date64, DataType::Date64) => {
+ cmp_dict::<$KT, Date64Type, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::YearMonth),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ cmp_dict::<$KT, IntervalYearMonthType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::DayTime),
+ DataType::Interval(IntervalUnit::DayTime),
+ ) => {
+ cmp_dict::<$KT, IntervalDayTimeType, _>($LEFT, $RIGHT, $OP)
+ }
+ (
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ ) => {
+ cmp_dict::<$KT, IntervalMonthDayNanoType, _>($LEFT, $RIGHT, $OP)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of value type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different value types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+macro_rules! typed_dict_compares {
+ // Applies `LEFT OP RIGHT` when `LEFT` and `RIGHT` both are `DictionaryArray`
+ ($LEFT: expr, $RIGHT: expr, $OP: expr) => {{
+ match ($LEFT.data_type(), $RIGHT.data_type()) {
+ (DataType::Dictionary(left_key_type, _), DataType::Dictionary(right_key_type, _))=> {
+ match (left_key_type.as_ref(), right_key_type.as_ref()) {
+ (DataType::Int8, DataType::Int8) => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let right = as_dictionary_array::<Int8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let right = as_dictionary_array::<Int16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let right = as_dictionary_array::<Int32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let right = as_dictionary_array::<Int64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let right = as_dictionary_array::<UInt8Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let right = as_dictionary_array::<UInt16Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let right = as_dictionary_array::<UInt32Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let right = as_dictionary_array::<UInt64Type>($RIGHT);
+ typed_dict_cmp!(left, right, $OP, UInt64Type)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing dictionary arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare two dictionary arrays of different key types ({} and {})",
+ t1, t2
+ ))),
+ }
+ }
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare dictionary array with non-dictionary array ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Helper function to perform boolean lambda function on values from two dictionary arrays, this
+/// version does not attempt to use SIMD.
+macro_rules! compare_dict_op {
+ ($left: expr, $right:expr, $op:expr, $value_ty:ty) => {{
+ if $left.len() != $right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+ let left_values = $left.values().as_any().downcast_ref::<$value_ty>().unwrap();
+ let right_values = $right
+ .values()
+ .as_any()
+ .downcast_ref::<$value_ty>()
+ .unwrap();
+
+ let result = $left
+ .keys()
+ .iter()
+ .zip($right.keys().iter())
+ .map(|(left_key, right_key)| {
+ if let (Some(left_k), Some(right_k)) = (left_key, right_key) {
+ let left_key = left_k.to_usize().expect("Dictionary index not usize");
+ let right_key =
+ right_k.to_usize().expect("Dictionary index not usize");
+ unsafe {
+ let left_value = left_values.value_unchecked(left_key);
+ let right_value = right_values.value_unchecked(right_key);
+ Some($op(left_value, right_value))
+ }
+ } else {
+ None
+ }
+ })
+ .collect();
+
+ Ok(result)
+ }};
+}
+
+/// Perform given operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
Review comment:
changed as suggested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1263: Implement DictionaryArray support in eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1263:
URL: https://github.com/apache/arrow-rs/pull/1263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org