You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/08/24 14:00:02 UTC
[GitHub] [arrow-rs] tustvold opened a new issue, #4733: Remove Deprecated Comparison Kernels
tustvold opened a new issue, #4733:
URL: https://github.com/apache/arrow-rs/issues/4733
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
<!--
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
(This section helps Arrow developers understand the context and *why* for this feature, in addition to the *what*)
-->
#4701 deprecated a large number of kernels in arrow-ord
**Describe the solution you'd like**
<!--
A clear and concise description of what you want to happen.
-->
Translate the existing tests to the new APIs, and remove the legacy kernels.
This should be a fairly mechanical exercise and so marking this as a good first issue
**Describe alternatives you've considered**
<!--
A clear and concise description of any alternative solutions or features you've considered.
-->
**Additional context**
<!--
Add any other context or screenshots about the feature request here.
-->
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Remove Deprecated Comparison Kernels [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed issue #4733: Remove Deprecated Comparison Kernels
URL: https://github.com/apache/arrow-rs/issues/4733
--
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] zinking commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1693006082
is this what you wanted
```
let r0: Vec<_> = crate::cmp::eq(
&Int64Array::from(vec![8, 8, 8, 8, 8, 8, 8, 8, 8, 8]),
&Int64Array::from(vec![6, 7, 8, 9, 10, 6, 7, 8, 9, 10])
).unwrap().values().iter().collect();
assert_eq!(
&vec![false, false, true, false, false, false, false, true, false, false],
&r0
);
cmp_i64!(
eq,
eq_dyn,
vec![8, 8, 8, 8, 8, 8, 8, 8, 8, 8],
vec![6, 7, 8, 9, 10, 6, 7, 8, 9, 10],
vec![false, false, true, false, false, false, false, true, false, false]
);
```
--
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] tustvold commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1693346661
> in my understanding all cmp now generates boolean arrays now
It is comparing two BooleanArray for equality, returning a single BooleanArray, why wouldn't this continue to apply?
--
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 issue #4733: Remove Deprecated Comparison Kernels
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1710141845
While working on https://github.com/apache/arrow-rs/issues/4788 I had an idea for porting the comparison kernel test that I think would be more satisfying than porting the existing tests as well as adding coverage.
# Proposal
Add "fuzz" testing to the comparison kernels that basically did:
1. Make a random inputs, and compute (using rust code) the expected output
2. Run the comparison kernel
3. Compare the output to the expected output
We could implement this fuzz testing in several PRs and then remove the old kernels
# Missing Coverage
I believe the existing kernels are missing the following coverage:
1. Tests for arrays with more than 64 values. I found [special case code](https://github.com/apache/arrow-rs/blob/6fdbc263b08fea0858b662e8d792f88268d9e741/arrow-ord/src/cmp.rs#L365-L398) for iterating through values 64 at a time (and then remainder) but I didn't see any tests that actually tested more than 64 values
2. Dictionary arrays where NULL inputs have invalid keys (the root cause of https://github.com/apache/arrow-rs/issues/4788)
--
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] tustvold commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1693009379
Yes, I would recommend porting the tests in their current location to make the diff easier to read, and then a follow on PR can move them to an integration test and delete the compare module
--
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] zinking commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1694530860
> Edit: are you calling BooleanArray::iter or BooleanArray::values().iter(). The latter will not yield nulls
looks good now. I was just thinking of Option should appear in the type system.
--
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] zinking commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1694239036
> A BooleanArray is inherently nullable, it is equivalent to `Vec<Option<bool>>`. This should just be a case of changing the method names, the semantics of the kernels have not changed?
meaning None translated to 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] zinking commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1694068325
> > in my understanding all cmp now generates boolean arrays now
>
> It is comparing two BooleanArray for equality, returning a single BooleanArray, why wouldn't this continue to apply?
it's actually returning Option[Boolean], or shall I just return false for None result ?
--
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] zinking commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1693243069
@tustvold
does this test still apply
```
#[test]
fn test_boolean_array_eq() {
let a: BooleanArray =
vec![Some(true), Some(false), Some(false), Some(true), Some(true), None]
.into();
let b: BooleanArray =
vec![Some(true), Some(true), Some(false), Some(false), None, Some(false)]
.into();
let res: Vec<Option<bool>> = eq_bool(&a, &b).unwrap().iter().collect();
assert_eq!(
res,
vec![Some(true), Some(false), Some(true), Some(false), None, None]
)
}
```
in my understanding all cmp now generates boolean arrays now
--
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] tustvold commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1694228812
A BooleanArray is inherently nullable, it is equivalent to `Vec<Option<bool>>`. This should just be a case of changing the method names, the semantics of the kernels have not changed?
--
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] tustvold commented on issue #4733: Remove Deprecated Comparison Kernels
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #4733:
URL: https://github.com/apache/arrow-rs/issues/4733#issuecomment-1694241362
> should generate compare result like [Some[true], Some[false], None]
They do, this ticket should largely be a fairly mechanical find and replace, I'm struggling to follow what you're asking...
--
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