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.

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

Enable casting to int64, uint64, and double in AST code. #9379

Merged
merged 8 commits into from
Oct 25, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 5, 2021

This PR resolves #8979, adding support for a few casting operators in AST code. These operators can be used to perform operations between columns with mismatched data types without materializing intermediates as new columns.

@vyasr vyasr added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Oct 5, 2021
@vyasr vyasr added this to the Conditional Joins milestone Oct 5, 2021
@vyasr vyasr self-assigned this Oct 5, 2021
@vyasr vyasr requested a review from a team as a code owner October 5, 2021 17:06
@vyasr vyasr requested review from trxcllnt and codereport October 5, 2021 17:06
@vyasr vyasr requested a review from jlowe October 5, 2021 17:06
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #9379 (b704f3a) into branch-21.12 (ab4bfaa) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9379      +/-   ##
================================================
+ Coverage         10.79%   10.83%   +0.04%     
================================================
  Files               116      117       +1     
  Lines             18869    19442     +573     
================================================
+ Hits               2036     2106      +70     
- Misses            16833    17336     +503     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_lib/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/_base_index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/datetime.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/lists.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/numerical.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py 0.00% <0.00%> (ø)
... and 76 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6caaf5...b704f3a. Read the comment docs.

Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

SFINAE and type traits.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Also add these operators to UnaryOperator.java:
https://github.com/rapidsai/cudf/blob/branch-21.12/java/src/main/java/ai/rapids/cudf/ast/UnaryOperator.java

Otherwise, looks good to me!

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@vyasr vyasr requested a review from a team as a code owner October 18, 2021 23:34
@github-actions github-actions bot added the Java Affects Java cuDF API. label Oct 18, 2021
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@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.

Since the Java code was updated as part of this PR to add bindings to these new AST operators, it would be nice to have Java tests to verify those bindings are operating correctly. Happy to do this as a followup if needed.

@vyasr
Copy link
Contributor Author

vyasr commented Oct 25, 2021

@jlowe if you don't mind adding the tests separately that would be great. It would be good to get this merged so I can work on some next steps in the AST code. If you don't get around to writing the tests let me know, I can always make the follow-up myself.

@vyasr
Copy link
Contributor Author

vyasr commented Oct 25, 2021

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Improved control over AST operator casting behavior
7 participants