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/10/31 11:21:36 UTC
[GitHub] [arrow-rs] alamb opened a new pull request, #2991: Fix ignored limit on `lexsort_to_indices`
alamb opened a new pull request, #2991:
URL: https://github.com/apache/arrow-rs/pull/2991
# Which issue does this PR close?
Closes limit is not applied for multi-column sorts lexsort_to_indices
# Rationale for this change
Regresssion was introduced in https://github.com/apache/arrow-rs/pull/2929 by https://github.com/apache/arrow-rs/pull/2929/files#r1005140128 and there was no test coverage ðŸ˜
# What changes are included in this PR?
1. Fix bug
2. Add test coverage
# Are there any user-facing changes?
Not really as we haven't released this code yet
--
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] isidentical commented on a diff in pull request #2991: Fix ignored limit on `lexsort_to_indices`
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991#discussion_r1009311276
##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -1422,6 +1422,18 @@ mod tests {
}
}
+ /// slace all arrays in expected_output to offset/length
Review Comment:
```suggestion
/// slice all arrays in expected_output to offset/length
```
--
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 #2991: Fix ignored limit on `lexsort_to_indices`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991#issuecomment-1297426366
I plan to create 26.0.0 RC2 with this fix
--
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] ursabot commented on pull request #2991: Fix ignored limit on `lexsort_to_indices`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991#issuecomment-1297433563
Benchmark runs are scheduled for baseline = 40d61ec07ad56e355205fe8dbcb074563e30c09e and contender = 66c9636742162f832b434a513769e158f9723e67. 66c9636742162f832b434a513769e158f9723e67 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/32489f8c68ba48edb22ede718bb2f6ac...9d6208963d054059af5379643ada3162/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/06e29a5c8ac544fb80bcea0892ad1f53...fa764d284f684d69bf056fbfb76b10ed/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d51ce72a733e49beb045afc9eddf42c4...01b408e940e2450dae7b83e208dff3bd/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/f2a97862a2a8445bb81dc0919851fad2...b70f98f0c3ba4820955b5cc444c0f5e4/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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 #2991: Fix ignored limit on `lexsort_to_indices`
Posted by GitBox <gi...@apache.org>.
alamb merged PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991
--
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 diff in pull request #2991: Fix ignored limit on `lexsort_to_indices`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991#discussion_r1009306085
##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3519,7 +3532,8 @@ mod tests {
Some(-2),
])) as ArrayRef,
];
- test_lex_sort_arrays(input, expected, None);
+ test_lex_sort_arrays(input.clone(), expected.clone(), None);
+ test_lex_sort_arrays(input, slice_arrays(expected, 0, 2), Some(2));
Review Comment:
This test fails immediately without the fix -- the output is too big!)
##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3439,7 +3451,8 @@ mod tests {
Some(2),
Some(17),
])) as ArrayRef];
- test_lex_sort_arrays(input.clone(), expected, None);
+ test_lex_sort_arrays(input.clone(), expected.clone(), None);
+ test_lex_sort_arrays(input.clone(), slice_arrays(expected, 0, 2), Some(2));
Review Comment:
This addition is strictly unnecessary from a coverage perspective (it was already covered), but I wanted to make the `test_lex_sort_arrays` based tests all consistently patterned so it was easier to reason about coverage
##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3439,7 +3451,8 @@ mod tests {
Some(2),
Some(17),
])) as ArrayRef];
- test_lex_sort_arrays(input.clone(), expected, None);
+ test_lex_sort_arrays(input.clone(), expected.clone(), None);
+ test_lex_sort_arrays(input.clone(), slice_arrays(expected, 0, 2), Some(2));
Review Comment:
The only place on master that a limit is passed to `lexsort_to_indices` in the tests is immediately below here. However, very sadly, there is a special case code path for single arrays that doesn't hit the bug path
```
let expected = vec![Arc::new(PrimitiveArray::<Int64Type>::from(vec![
Some(-1),
Some(0),
Some(2),
])) as ArrayRef];
test_lex_sort_arrays(input, expected, Some(3));
```
##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -950,7 +950,7 @@ pub fn lexsort_to_indices(
});
Ok(UInt32Array::from_iter_values(
- value_indices.iter().map(|i| *i as u32),
+ value_indices.iter().take(len).map(|i| *i as u32),
Review Comment:
this is the bugfix
--
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