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

Replace toTitle with capitalize for GpuInitCap #2838

Merged
merged 15 commits into from
Jul 7, 2021
Merged

Replace toTitle with capitalize for GpuInitCap #2838

merged 15 commits into from
Jul 7, 2021

Conversation

firestarman
Copy link
Collaborator

Replace toTitle with capitalize for GpuInitCap to align with the behavior of Spark InitCap.

closes #2786
closes #120

Signed-off-by: Firestarman [email protected]

@firestarman firestarman changed the title Replace toTitle with capitalize for GpuInitCap [WIP]Replace toTitle with capitalize for GpuInitCap Jun 29, 2021
@firestarman firestarman changed the title [WIP]Replace toTitle with capitalize for GpuInitCap [WIP] Replace toTitle with capitalize for GpuInitCap Jun 29, 2021
@firestarman
Copy link
Collaborator Author

firestarman commented Jun 29, 2021 •
edited
Loading

This PR depends on rapidsai/cudf#8624, and it is WIP, because the doc can not be updated due to build failures.

@firestarman firestarman self-assigned this Jun 29, 2021
@firestarman firestarman added cudf_dependency An issue or PR with this label depends on a new feature in cudf bug Something isn't working labels Jun 29, 2021
@firestarman
Restore the comment
689569a 
Signed-off-by: Firestarman <[email protected]>
@firestarman
more comment
f8c67ed 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Address the comments
f2a1e2a 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Longer size for string gen
cc1c353 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Address new comments
1e30a96 
Signed-off-by: Firestarman <[email protected]>
@firestarman
comment update
dd07267 
Signed-off-by: Firestarman <[email protected]>
jlowe
jlowe reviewed Jul 1, 2021
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

I thought we should cover at least some of the characters mentioned in the bug

Yes, many characters are now covered. IIUC the only issue is that there are some corner-case characters that could be mishandled based on characters that were added to the Unicode standard between versions. If cudf and JVM aren't targeting the same version of the Unicode standard then those characters occurring in the string can cause a mismatch between GPU and CPU output.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala Outdated Show resolved Hide resolved
firestarman and others added 4 commits July 2, 2021 09:53
@firestarman @jlowe
Update sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides…
17f88c9 
….scala


Improve the incompat doc

Co-authored-by: Jason Lowe <[email protected]>
@firestarman
More characters
613284f 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Doc update
188e409 
Signed-off-by: Firestarman <[email protected]>
@firestarman
less characters.
a337527 
Since tests failed due to some special charaters

Signed-off-by: Firestarman <[email protected]>
@firestarman firestarman changed the title [WIP] Replace toTitle with capitalize for GpuInitCap Replace toTitle with capitalize for GpuInitCap Jul 2, 2021
@firestarman firestarman marked this pull request as ready for review July 2, 2021 06:00
@firestarman
Copy link
Collaborator Author

build

@firestarman
Copy link
Collaborator Author

firestarman commented Jul 2, 2021 •
edited
Loading

Filed the bug rapidsai/cudf#8644 to track the failures for capitalize with some special characters .
Seems the fix rapidsai/cudf#4520 for rapidsai/cudf#3132 only works for upper and lower.

@firestarman
doc update
63d02d4 
Signed-off-by: Firestarman <[email protected]>
@firestarman
correct the doc
f8916ff 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Add isssue link in the doc
3ac3e81 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Copy link
Collaborator Author

build

@firestarman
Addressed the new comments.
24ef719 
Signed-off-by: Firestarman <[email protected]>
@firestarman
Copy link
Collaborator Author

build

@jlowe jlowe merged commit 7f524d9 into NVIDIA:branch-21.08 Jul 7, 2021
@firestarman firestarman deleted the initcap branch July 8, 2021 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe approved these changes

Assignees

@firestarman firestarman

Labels
bug Something isn't working cudf_dependency An issue or PR with this label depends on a new feature in cudf
Projects
None yet
Milestone
No milestone
2 participants
@firestarman @jlowe