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 2021/12/22 17:42:46 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
alamb opened a new pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475
# Which issue does this PR close?
Marking as draft because the PR fails as the Arrow kernels don't handle timestamps and dates, which DataFusion does. I will make a PR to fix arrow shortly
re https://github.com/apache/arrow-datafusion/issues/1474
# Rationale for this change
@Jimexist added dyn kernels in https://github.com/apache/arrow-rs/pull/858 but we still have the dispatch logic in datafusion, which is redundant (and as we add more logic to arrow-rs we will need to change datafusion to take advantage of it)
@matthewmturner is working on a similar set of kernels for scalars in https://github.com/apache/arrow-rs/pull/1074
# What changes are included in this PR?
Use different arrow kernels
# Are there any user-facing changes?
No
--
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-datafusion] alamb merged pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475
--
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-datafusion] liukun4515 commented on a change in pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#discussion_r808919223
##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -91,8 +131,10 @@ fn is_not_distinct_from_bool(
.collect())
}
-// TODO add iter for decimal array
-// TODO move this to arrow-rs
+// TODO move decimal kernels to to arrow-rs
Review comment:
Maybe I can take this, and migrate decimal to arrow-rs kernel recently
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1042934538
> @alamb codebase is becoming more cleaner.
Thanks -- I am trying @liukun4515 . What I really hope to do with these kernels is get to a point where we can add more specialized implementations in arrow-rs and have them flow automatically to DataFusion.
A particular interest of ours in IOx is performance of dictionary encoded data -- so after this PR is merged, the work that @viirya is doing in PRs like https://github.com/apache/arrow-rs/pull/1326 will end up automatically being available in DataFusion)
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1040735096
cc @matthewmturner @viirya and @liukun4515 I think this is now ready for review
--
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-datafusion] viirya commented on a change in pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#discussion_r809291292
##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -1194,12 +1236,12 @@ impl BinaryExpr {
match &self.op {
Operator::Like => binary_string_array_op!(left, right, like),
Operator::NotLike => binary_string_array_op!(left, right, nlike),
- Operator::Lt => binary_array_op!(left, right, lt),
- Operator::LtEq => binary_array_op!(left, right, lt_eq),
- Operator::Gt => binary_array_op!(left, right, gt),
- Operator::GtEq => binary_array_op!(left, right, gt_eq),
- Operator::Eq => binary_array_op!(left, right, eq),
- Operator::NotEq => binary_array_op!(left, right, neq),
+ Operator::Lt => lt_dyn(&left, &right),
+ Operator::LtEq => lt_eq_dyn(&left, &right),
+ Operator::Gt => gt_dyn(&left, &right),
+ Operator::GtEq => gt_eq_dyn(&left, &right),
+ Operator::Eq => eq_dyn(&left, &right),
+ Operator::NotEq => neq_dyn(&left, &right),
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-datafusion] liukun4515 commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1000859993
> > @alamb This pr #1483 may be ready next week.
> > After comparison the decimal pull request, I will file the pull request about the mathematics operation with decimal type.
> > I think we can finish the feature of binary operation with decimal first.
>
> 👍 sounds like a great plan to me.
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1001184214
@liukun4515 I am hopefuly that eventually the `dyn_eq` / `dyn_eq_lit` type kernels that @matthewmturner is working on in https://github.com/apache/arrow-rs/pull/1074 will also support `Decimal` values (though we will have to work out precision and scale)
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1015511151
This PR is failing because eq_dyn kernels don't have support for Decimal. I will add that
--
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-datafusion] liukun4515 commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1000768726
@alamb This pr https://github.com/apache/arrow-datafusion/pull/1483 may be ready next week.
After comparison the decimal pull request, I will file the pull request about the mathematics operation with decimal type.
I think we can finish the feature of binary operation with decimal first.
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1006533716
Rebased on top of https://github.com/apache/arrow-datafusion/pull/1523 (aka upgrade to arrow 7.0.0) which includes https://github.com/apache/arrow-rs/pull/1095 from @viirya ❤️ and this PR is in much better shape.
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1018556979
Switching back to draft until the underlying arrow kernel support is done
--
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-datafusion] liukun4515 commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1001511232
> @liukun4515 I am hopefuly that eventually the `dyn_eq` / `dyn_eq_lit` type kernels that @matthewmturner is working on in [apache/arrow-rs#1074](https://github.com/apache/arrow-rs/pull/1074) will also support `Decimal` values (though we will have to work out precision and scale)
The https://github.com/apache/arrow-datafusion/pull/1483 is ready for merged, now I am developing the kernel for decimal operation in the arrow-rs. I think it will ready in the next release of arrow-rs.
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1000793977
> @alamb This pr #1483 may be ready next week.
> After comparison the decimal pull request, I will file the pull request about the mathematics operation with decimal type.
> I think we can finish the feature of binary operation with decimal first.
👍 sounds like a great plan to me.
--
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-datafusion] alamb commented on pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#issuecomment-1015636931
Filed https://github.com/apache/arrow-rs/issues/1200 to track adding support for decimal arrays
--
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-datafusion] liukun4515 commented on a change in pull request #1475: Use`eq_dyn`, `neq_dyn`, `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` kernels from arrow
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1475:
URL: https://github.com/apache/arrow-datafusion/pull/1475#discussion_r808918039
##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -91,8 +131,10 @@ fn is_not_distinct_from_bool(
.collect())
}
-// TODO add iter for decimal array
-// TODO move this to arrow-rs
+// TODO move decimal kernels to to arrow-rs
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