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/01 04:43:44 UTC

[GitHub] [arrow-rs] matthewmturner opened a new pull request #1115: Add lt eq dyn scalar kernel

matthewmturner opened a new pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115


   # Which issue does this PR close?
   
   Closes #1113 task `lt_eq_dyn_scalar` 
   
   # 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] alamb commented on a change in pull request #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777196959



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       @matthewmturner  your assessment is correct.
   
   I think supporting `Float32Array` and `Float64Array` would be reasonable here.
   
   As you say, however, the use of the intermediate `i128` makes using `f32` and `f64` as the constant values not possible without a loss of precision.
   
   🤔  we should probably file a ticket to sort this out (probably via some sort of trait).
   




-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#issuecomment-1003701060


   I think the "multiple PR idea" may not have been ideal here as they ended up conflicting with each other :) -- I'll tend to these PRs and get them merged in


-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115


   


-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777220989



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       Here is what I came up with: https://github.com/apache/arrow-rs/pull/1127
   
   I am pleased with the result -- I need to polish up just a little bit more




-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#issuecomment-1003505158


   # [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1115?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 [#1115](https://codecov.io/gh/apache/arrow-rs/pull/1115?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9ff648e) into [master](https://codecov.io/gh/apache/arrow-rs/commit/2ad99ec33c757d6c7b4a5e69e19a4a096813b53b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2ad99ec) will **decrease** coverage by `0.02%`.
   > The diff coverage is `75.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-rs/pull/1115/graphs/tree.svg?width=650&height=150&src=pr&token=pq9V9qWZ1N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-rs/pull/1115?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    #1115      +/-   ##
   ==========================================
   - Coverage   82.34%   82.32%   -0.03%     
   ==========================================
     Files         168      168              
     Lines       49479    49631     +152     
   ==========================================
   + Hits        40743    40858     +115     
   - Misses       8736     8773      +37     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1115?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/1115/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) | `90.56% <75.00%> (-2.92%)` | :arrow_down: |
   | [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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.38% <0.00%> (-0.43%)` | :arrow_down: |
   | [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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: |
   | [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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.68% <0.00%> (+0.30%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1115?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/1115?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 [2ad99ec...9ff648e](https://codecov.io/gh/apache/arrow-rs/pull/1115?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] matthewmturner commented on a change in pull request #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777169804



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       its a good question and i had thought about it recently as well but expected to address after these were merged.
   
   below is my rough understanding but ill leave it to @alamb to confirm / expand on this.
   
   we are leveraging existing kernels, such as `eq_scalar` which take `PrimitiveArray` and `ArrowNumericType::Native` which i believe is a `ArrowNativeType` (https://docs.rs/arrow/latest/arrow/datatypes/trait.ArrowNativeType.html).  Both `f32` and `f64` impl this trait and of course there can be `Float32` and `Float64` `PrimitiveArray`s so i think technically we could (however i dont see any tests for using the kernels we are leveraging with decimal values).   right now were kind of using a hack though by casting to a `i128` as a catch all and then back to the appropriate rust / arrow type to use the attendant kernel.  my guess is if we were comfortable casting to `f64` as the catch all instead of `i128` then we could simply add support for those data types but i admit to being new to the tradeoffs, such as precision, that may arise from using `f64` over `i128` so will defer to @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] liukun4515 commented on a change in pull request #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777111750



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       I have a question about the data type.
   Why not implement and support the FLOAT32, FLOAT64.




-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#issuecomment-1003700558


   I merged from master and will merge this PR after a clean CI run


-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#issuecomment-1003505158


   # [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1115?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 [#1115](https://codecov.io/gh/apache/arrow-rs/pull/1115?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (815ed69) into [master](https://codecov.io/gh/apache/arrow-rs/commit/0d825c196e343805c7500bdc06af0c6e941e2577?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d825c1) will **increase** coverage by `0.00%`.
   > The diff coverage is `85.18%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-rs/pull/1115/graphs/tree.svg?width=650&height=150&src=pr&token=pq9V9qWZ1N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-rs/pull/1115?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    #1115   +/-   ##
   =======================================
     Coverage   82.32%   82.32%           
   =======================================
     Files         168      168           
     Lines       49577    49631   +54     
   =======================================
   + Hits        40812    40857   +45     
   - Misses       8765     8774    +9     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1115?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/1115/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) | `90.56% <85.18%> (-0.32%)` | :arrow_down: |
   | [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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.37% <0.00%> (-0.31%)` | :arrow_down: |
   | [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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.23%)` | :arrow_down: |
   | [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1115/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: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1115?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/1115?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 [0d825c1...815ed69](https://codecov.io/gh/apache/arrow-rs/pull/1115?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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777198799



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       It looks to me like https://docs.rs/num/latest/num/trait.ToPrimitive.html `ToPrimitive` may be the correct trait in this case. Let me spike out what that would look like




-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777108162



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1157,6 +1157,42 @@ where
     }
 }
 
+/// Perform `left <= right` operation on an array and a numeric scalar
+/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
+pub fn lt_eq_dyn_scalar<T>(left: Arc<dyn Array>, right: T) -> Result<BooleanArray>
+where
+    T: TryInto<i128> + Copy + std::fmt::Debug,
+{
+    match left.data_type() {
+        DataType::Dictionary(key_type, value_type) => match value_type.as_ref() {
+            DataType::Int8
+            | DataType::Int16
+            | DataType::Int32
+            | DataType::Int64
+            | DataType::UInt8
+            | DataType::UInt16
+            | DataType::UInt32
+            | DataType::UInt64 => {dyn_compare_scalar!(&left, right, key_type, lt_eq_scalar)}
+            _ => Err(ArrowError::ComputeError(
+                "Kernel only supports PrimitiveArray or DictionaryArray with Primitive values".to_string(),

Review comment:
       also here I think having the actual name of the kernel would help usability quite a lot




-- 
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 #1115: Add lt eq dyn scalar kernel

Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1115:
URL: https://github.com/apache/arrow-rs/pull/1115#discussion_r777161312



##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -888,6 +890,375 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
     compare_op_scalar!(left, right, |a, b| a >= b)
 }
 
+macro_rules! dyn_compare_scalar {
+    // Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray`
+    ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+        let right: i128 = $RIGHT.try_into().map_err(|_| {
+            ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+        })?;
+        match $LEFT.data_type() {
+            DataType::Int8 => {

Review comment:
       @matthewmturner @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