Support MERGE_LISTS and MERGE_SETS in Java package [skip ci] #8516
Conversation
Signed-off-by: sperlingxx <[email protected]>
sperlingxx
added
non-breaking
sperlingxx
changed the title
Co-authored-by: Nghia Truong <[email protected]>
|
@gpucibot merge |
|
@sperlingxx did you test this? With this patch I am getting all kinds of errors with the lead/lag tests that don't exist before the patch. |
| @@ -56,10 +56,12 @@ | |||
| ROW_NUMBER(17), | |||
| COLLECT_LIST(18), | |||
| COLLECT_SET(19), | |||
| LEAD(20), | |||
There was a problem hiding this comment.
@revans2 I assume that your problem is due to this.
There was a problem hiding this comment.
No my problem is that @sperlingxx didn't even run the unit tests or he would have found this and there would have been no issue. I will post a patch shortly
There was a problem hiding this comment.
Here we arbitrarily define the value for aggregation enums, which is different from the enums defined in libcudf. I was talking about this before (probably with Jason). We should have some better way to automate this. Otherwise, we will continue to have issues with this in the future.
There was a problem hiding this comment.
These just happen to match the values in libcudf. But in this case we never cast them so it does not have to match. This is the one place that we do it correctly. If we did cast them, then your patch that changed the values would have broken the java build, not this patch.
There was a problem hiding this comment.
Oh, it's my mistake! I apologized for my carelessness.
Fixes an issue introduced by #8516 Authors: - Robert (Bobby) Evans (https://github.com/revans2) Approvers: - Jason Lowe (https://github.com/jlowe) URL: #8600
Closes #8445
This PR is to provide Java bindings for #8436.