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 2020/05/25 02:51:52 UTC
[GitHub] [arrow] houqp opened a new pull request #7265: ARROW-8906: [Rust] add lexical sort support to arrow compute kernel
houqp opened a new pull request #7265:
URL: https://github.com/apache/arrow/pull/7265
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me closed pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #7265:
URL: https://github.com/apache/arrow/pull/7265
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#discussion_r430259731
##########
File path: rust/arrow/src/compute/kernels/sort.rs
##########
@@ -668,4 +830,300 @@ mod tests {
],
);
}
+
+ #[test]
+ fn test_lex_sort_single_column() {
+ let input = vec![SortColumn {
+ values: Arc::new(PrimitiveArray::<Int64Type>::from(vec![
+ Some(17),
Review comment:
I got stuck while working on #7193, there'll be merge conflicts between that and this PR, so if this one gets merged first, I'll attempt adding `f64;NAN` when I rebase
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#issuecomment-636511382
> For those who are interested, here is the datafusion sort expression support commit, which depends on this PR: [houqp@3fbe393](https://github.com/houqp/arrow/commit/3fbe393c6b6718a70028d311d659e506415666db)
This looks great, especially the `concat` kernel. I'll merge this PR tomorrow if there isn't another review, we can always address feedback in subsequent JIRAs.
I also updated the float-sort in #7193, may you please also look at it and see if we'd need to change anything to support floats in lexsort?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#issuecomment-633361905
https://issues.apache.org/jira/browse/ARROW-8931
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#discussion_r430064815
##########
File path: rust/arrow/src/compute/kernels/sort.rs
##########
@@ -668,4 +830,300 @@ mod tests {
],
);
}
+
+ #[test]
+ fn test_lex_sort_single_column() {
+ let input = vec![SortColumn {
+ values: Arc::new(PrimitiveArray::<Int64Type>::from(vec![
+ Some(17),
Review comment:
@andygrove this PR currently doesn't support float array due to `Ord` trait constraints. I recommend adding float support as a follow up after we finalize the design in https://github.com/apache/arrow/pull/7193.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#discussion_r430044935
##########
File path: rust/arrow/src/compute/kernels/sort.rs
##########
@@ -668,4 +830,300 @@ mod tests {
],
);
}
+
+ #[test]
+ fn test_lex_sort_single_column() {
+ let input = vec![SortColumn {
+ values: Arc::new(PrimitiveArray::<Int64Type>::from(vec![
+ Some(17),
Review comment:
Could you add `f64::NAN` to the values being tested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#issuecomment-636509592
For those who are interested, here is the datafusion sort expression support commit, which depends on this PR: https://github.com/houqp/arrow/commit/3fbe393c6b6718a70028d311d659e506415666db
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7265: ARROW-8906: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#issuecomment-633352999
https://issues.apache.org/jira/browse/ARROW-8906
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #7265: ARROW-8931: [Rust] add lexical sort support to arrow compute kernel
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #7265:
URL: https://github.com/apache/arrow/pull/7265#issuecomment-636549777
> I also updated the float-sort in #7193, may you please also look at it and see if we'd need to change anything to support floats in lexsort?
Based on your implementation in #7193, looks like it should be pretty straight forward to add float support to lexsort. We can implement the unwrap to Greater logic in `cmp_value` and relax the Ord constraint for OrdArray. Alternatively, we can change OrdArray into PartialOrdArray that returns `Option<Ordering>`, then add the default to Greater logic into `lexsort` function. Either way will work with minimal code change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org