ARROW-4990: [C++] Support Array-Array comparison #4398
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fsaintjacques
force-pushed
the
ARROW-4990-compare-array-array
branch
from
5c47b82 to
6cfef25
Compare
emkornfield
reviewed
May 30, 2019
Contributor
There was a problem hiding this comment.
does it pay to support scalar-scalar comparison (i.e. should this be 1)?
Member
There was a problem hiding this comment.
I think we can support comparing scalars by promoting Scalar to Array of length 1. Is there a JIRA about Scalar->Array promotion?
Contributor
Author
There was a problem hiding this comment.
emkornfield
reviewed
May 30, 2019
Contributor
There was a problem hiding this comment.
Is this different then AssignNullIntersection?
Contributor
Author
There was a problem hiding this comment.
it absolutely is not, didn't know why I missed this one. I'll refactor do call AssignNullIntersection or remove it?
Contributor
There was a problem hiding this comment.
I think refactoring to call AssignNullIntersection sounds good, since I clearly didn't pick a good name.
Contributor
Author
There was a problem hiding this comment.
It's more that I didn't look what was there than name picking. Even worse, I think I even reviewed your function...
Contributor
|
Took a quick pass on this, will look at it in more detail tomorrow. |
emkornfield
reviewed
May 31, 2019
Contributor
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
Looks like appveyor is broken with something related.
Contributor
There was a problem hiding this comment.
I think refactoring to call AssignNullIntersection sounds good, since I clearly didn't pick a good name.
Contributor
There was a problem hiding this comment.
not this change, but I think the convention we should be using for now is that we use detail::Invoke to handle chunked arrays in the top level method, and worry about the parallelization when we expose the raw kernels? Thoughts?
Contributor
Author
There was a problem hiding this comment.
I agree but I suspect it will be hard to make with the current Invoke method, as the shape can vary, e.g. the signature is (S,T) -> R, where S,T,R can be Array, ChunkedArray, Scalar.
Contributor
There was a problem hiding this comment.
Hmm, so I think the complication is scalar. (I thought invoke handled combinations of Array and ChunkedArray). One way of potentially doing this is converting the Kernel into a Unary one when the function is called with scalar and use the InvokeUnary instead (and handle scalar to scalar here).
fsaintjacques
force-pushed
the
ARROW-4990-compare-array-array
branch
from
b97c787 to
8c3d1de
Compare
Contributor
Author
|
@emkornfield updated with comments. I filled https://issues.apache.org/jira/browse/ARROW-5489 |
emkornfield
reviewed
Jun 4, 2019
Contributor
There was a problem hiding this comment.
this seems strange why the r-value reference?
Contributor
Author
There was a problem hiding this comment.
Because of https://github.com/apache/arrow/pull/4398/files#diff-4cce1db37d9827bc657384ea82da0d63R42 . I'll modify the code to avoid the r-value.
Contributor
|
1 small nit about r-value reference that I don't understand, otherwise LGTM. |
Contributor
Author
|
@ursabot build |
|
I've successfully started builds for this PR |
Contributor
Author
|
@emkornfield I changed the forward to a pointer as per comment and style guide. |
Convenience method for sub-type supporting the length method.
When given 2 arrays, it should compose the validity bitmap by intersecting the input validity bitmaps.
- Extend CompareKernel to support (Array, Array) -> Array signature. - Extend CompareKernel to support (Scalar, Array) -> Array signature (swap the order of inputs).
fsaintjacques
force-pushed
the
ARROW-4990-compare-array-array
branch
from
8fafd6d to
864c679
Compare
Contributor
Author
|
@ursabot benchmark |
|
I've successfully started builds for this PR |
|
AMD64 Ubuntu 18.04 C++ Benchmark (#14985) builder failed. Revision: 864c679 Archery: |
Codecov Report
@@ Coverage Diff @@
## master #4398 +/- ##
===========================================
- Coverage 88.26% 76.92% -11.35%
===========================================
Files 846 51 -795
Lines 103360 1976 -101384
Branches 1253 0 -1253
===========================================
- Hits 91231 1520 -89711
+ Misses 11882 456 -11426
+ Partials 247 0 -247Continue to review full report at Codecov.
|
Contributor
|
+1, LGTM. It looks like the Ursabot output could be more useful |
Contributor
Author
|
I'm fixing this with Kristian. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Comparison only supported for the left argument to be an array and the right argument a scalar. This extends support for comparing two arrays, but also supporting the case where the left argument is a scalar and the right an array.