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/01/04 03:36:19 UTC
[GitHub] [arrow-rs] matthewmturner opened a new pull request #1131: Add dyn boolean kernels
matthewmturner opened a new pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131
# Which issue does this PR close?
Closes #1113 task dyn bool kernels
# 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] codecov-commenter edited a comment on pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#issuecomment-1004508471
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1131?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 [#1131](https://codecov.io/gh/apache/arrow-rs/pull/1131?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (de43f64) into [master](https://codecov.io/gh/apache/arrow-rs/commit/d8522298d27ee475602e4ae7bf8b5756750614f2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d852229) will **increase** coverage by `0.00%`.
> The diff coverage is `79.36%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1131?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 #1131 +/- ##
=======================================
Coverage 82.33% 82.33%
=======================================
Files 169 169
Lines 50055 50115 +60
=======================================
+ Hits 41213 41264 +51
- Misses 8842 8851 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1131?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/1131/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) | `89.44% <79.36%> (-0.31%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1131/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%> (+0.22%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1131?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/1131?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 [d852229...de43f64](https://codecov.io/gh/apache/arrow-rs/pull/1131?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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778544484
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -3599,4 +3674,59 @@ mod tests {
BooleanArray::from(vec![Some(false), Some(true), Some(false)])
);
}
+
+ #[test]
+ fn test_lt_dyn_bool_scalar() {
Review comment:
`NULL > FALSE ` -> `NULL`
We should make sure this case is right.
--
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 #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779111464
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
Review comment:
I think we should update this to
```suggestion
pub fn lt_dyn_bool_scalar(left: &dyn Array, right: bool) -> Result<BooleanArray> {
```
Rather than taking an owned `Arc` to be consistent with the work in https://github.com/apache/arrow-rs/pull/1127
I am happy to make the change as a follow on PR as well.
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
Review comment:
I don't think these kernels actually support `DictionaryArray` do they? I also don't think that it is needed -- maybe we can just update the docstrings?
--
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] matthewmturner commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779123373
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
Review comment:
since this PR is for bool i think best to update the signatures in a follow on PR. to confirm though, this would be for all dyn scalar kernels right? scalar, utf8, and bool?
--
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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778511124
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1454,6 +1454,81 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
result
}
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
+ let result = match left.data_type() {
+ DataType::Boolean => {
+ let left = as_boolean_array(&left);
+ lt_bool_scalar(left, right)
+ }
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports BooleanArray".to_string(),
Review comment:
Same comments for the below functions
--
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] matthewmturner commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779132816
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
Review comment:
sure sounds good
--
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] matthewmturner commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778536229
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -3599,4 +3674,59 @@ mod tests {
BooleanArray::from(vec![Some(false), Some(true), Some(false)])
);
}
+
+ #[test]
+ fn test_lt_dyn_bool_scalar() {
Review comment:
sure - but can you expand on why? is there a specific concern on this test with 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] alamb commented on pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#issuecomment-1006107853
Thanks again @matthewmturner
--
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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778511527
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -3599,4 +3674,59 @@ mod tests {
BooleanArray::from(vec![Some(false), Some(true), Some(false)])
);
}
+
+ #[test]
+ fn test_lt_dyn_bool_scalar() {
Review comment:
Can you add the `null` element in your test?
--
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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778544484
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -3599,4 +3674,59 @@ mod tests {
BooleanArray::from(vec![Some(false), Some(true), Some(false)])
);
}
+
+ #[test]
+ fn test_lt_dyn_bool_scalar() {
Review comment:
`NULL > FALSE ` -> `NULL`.
If left or right is `NULL`, the result must be `NULL`.
We should make sure this case is right.
--
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] liukun4515 commented on pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#issuecomment-1004585486
It's great, all the logic operations are in the pull request.
--
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 #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#issuecomment-1004508471
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1131?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 [#1131](https://codecov.io/gh/apache/arrow-rs/pull/1131?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c857dcb) into [master](https://codecov.io/gh/apache/arrow-rs/commit/d8522298d27ee475602e4ae7bf8b5756750614f2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d852229) will **increase** coverage by `0.00%`.
> The diff coverage is `83.33%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1131?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 #1131 +/- ##
=======================================
Coverage 82.33% 82.34%
=======================================
Files 169 169
Lines 50055 50115 +60
=======================================
+ Hits 41213 41266 +53
- Misses 8842 8849 +7
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1131?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/1131/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) | `89.44% <83.33%> (-0.31%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1131/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=) | `65.98% <0.00%> (ø)` | |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1131/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=) | `85.69% <0.00%> (+0.13%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1131/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.71% <0.00%> (+0.19%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1131/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.80% <0.00%> (+0.42%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1131?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/1131?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 [d852229...c857dcb](https://codecov.io/gh/apache/arrow-rs/pull/1131?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 #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779124710
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
Review comment:
> since this PR is for bool i think best to update the signatures in a follow on PR. to confirm though, this would be for all dyn scalar kernels right? scalar, utf8, and bool?
Yes
However, I made the change for `scalar` and `utf8` already in https://github.com/apache/arrow-rs/pull/1127 -- so how about I merge this PR and then I can make the updates to that PR as well
--
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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778510827
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1454,6 +1454,81 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
result
}
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
+ let result = match left.data_type() {
+ DataType::Boolean => {
+ let left = as_boolean_array(&left);
+ lt_bool_scalar(left, right)
+ }
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports BooleanArray".to_string(),
Review comment:
It's better to add the operation name in the error message.
--
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] liukun4515 commented on a change in pull request #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r778544484
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -3599,4 +3674,59 @@ mod tests {
BooleanArray::from(vec![Some(false), Some(true), Some(false)])
);
}
+
+ #[test]
+ fn test_lt_dyn_bool_scalar() {
Review comment:
`NULL > FALSE ` -> `NULL`
--
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 #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131#discussion_r779166488
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1448,7 +1448,82 @@ pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanAr
eq_bool_scalar(left, right)
}
_ => Err(ArrowError::ComputeError(
- "Kernel only supports BooleanArray".to_string(),
+ "eq_dyn_bool_scalar only supports BooleanArray".to_string(),
+ )),
+ };
+ result
+}
+
+/// Perform `left < right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn lt_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
Review comment:
Updated API in https://github.com/apache/arrow-rs/pull/1127/commits/e97f588a98efccd18e435a00a4e89841f010c94a
--
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 #1131: Add dyn boolean kernels
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1131:
URL: https://github.com/apache/arrow-rs/pull/1131
--
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