Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Java] Add JNI for substring without 'end' parameter. #12113

Merged
merged 5 commits into from
Nov 11, 2022
Merged

[Java] Add JNI for substring without 'end' parameter. #12113

merged 5 commits into from
Nov 11, 2022

Conversation

firestarman
Copy link
Contributor

@firestarman firestarman commented Nov 10, 2022 •
edited
Loading

cudf substring(column, int, int) has its own behavior when end is -1 (a valid position pointing to the last char), which is different from that in JNI. So JNI should not override it.

Besides, JNI's behavior does not match its doc when end is "-1". According to the doc, the last char should not be included in the result substring. But currently it does.

Signed-off-by: Liangcai Li [email protected]

Description

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@firestarman
Add JNI for substring without 'end' parameter.
48ae911 
cudf substring has its own behavior when end is -1, which is different
from that in JNI, so JNI should not override it.

Signed-off-by: Liangcai Li <[email protected]>
@firestarman firestarman requested a review from a team as a code owner November 10, 2022 08:52
@github-actions github-actions bot added the Java Affects Java cuDF API. label Nov 10, 2022
@firestarman firestarman added bug Something isn't working 3 - Ready for Review Ready for review by team breaking Breaking change labels Nov 10, 2022
@firestarman firestarman mentioned this pull request Nov 10, 2022
Merged
@firestarman
Copy link
Contributor Author

firestarman commented Nov 10, 2022 •
edited
Loading

NOTE: This is a breaking change, and I have filed PR NVIDIA/spark-rapids#7040 for the plugin.
This PR will change the behavior of ColumnView.substring when setting end to -1.

@HaoYang670
Copy link
Contributor

which is different from that in JNI, so JNI should not override it.

Could you please describe the behaviour you want in the PR description?
Besides, as this is a breaking change (not a bug), could you please explain why you can't do the change in spark-plugin, but in cudf?

@firestarman
fix a style issue
5b92a09 
Signed-off-by: Liangcai Li <[email protected]>
@firestarman
Copy link
Contributor Author

firestarman commented Nov 10, 2022 •
edited
Loading

Could you please describe the behaviour you want in the PR description? Besides, as this is a breaking change (not a bug), could you please explain why you can't do the change in spark-plugin, but in cudf?

The one described in the doc of substring is what I want. I think it is a bug in JNI, so I changed it here instead of plugin.

@codecov
Copy link

codecov bot commented Nov 10, 2022 •
edited
Loading

Codecov Report

Base: 88.06% // Head: 88.09% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (240ab89) compared to base (59bd5c3).
Patch has no changes to coverable lines.

❗ Current head 240ab89 differs from pull request most recent head e32893b. Consider uploading reports for the commit e32893b to get more accurate results

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.12   #12113      +/-   ##
================================================
+ Coverage         88.06%   88.09%   +0.03%     
================================================
  Files               135      135              
  Lines             22101    22100       -1     
================================================
+ Hits              19463    19469       +6     
+ Misses             2638     2631       -7
Impacted Files Coverage Δ
python/cudf/cudf/core/_compat.py 100.00% <ø> (ø)
python/cudf/cudf/core/dataframe.py 93.67% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.65% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.51% <0.00%> (+0.20%) ⬆️
python/cudf/cudf/core/column/numerical.py 95.46% <0.00%> (+0.28%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 93.75% <0.00%> (+0.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

revans2
revans2 approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me but it would be nice to address the comments by @ttnghia

@firestarman firestarman added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Nov 11, 2022
@firestarman
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3894427 into rapidsai:branch-22.12 Nov 11, 2022
@firestarman firestarman deleted the jni-substring branch November 11, 2022 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttnghia ttnghia ttnghia approved these changes

@revans2 revans2 revans2 approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team 5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change bug Something isn't working Java Affects Java cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@firestarman @HaoYang670 @revans2 @ttnghia