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

Support null literals in expressions #9117

Merged
merged 9 commits into from
Sep 7, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Aug 25, 2021

This PR resolves #8831 by propagating null values appropriately when they are provided in the form of literals. In addition to adding support at the level of expression evaluation, this PR also adds the appropriate dispatch to the correct code path at the level of calls to APIs using expressions containing nulls so that the null-supporting code paths can be triggered even when operating on tables that do not contain any nulls.

@vyasr vyasr added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. non-breaking Non-breaking change labels Aug 25, 2021
@vyasr vyasr added this to the Conditional Joins milestone Aug 25, 2021
@vyasr vyasr self-assigned this Aug 25, 2021
@vyasr vyasr requested review from a team as code owners August 25, 2021 19:24
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.

Tiny nit but otherwise this looks good from the Java perspective.

cpp/src/ast/expression_parser.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@f0fa255). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 6f1008c differs from pull request most recent head 28c2d73. Consider uploading reports for the commit 28c2d73 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9117   +/-   ##
===============================================
  Coverage                ?   10.77%           
===============================================
  Files                   ?      115           
  Lines                   ?    18722           
  Branches                ?        0           
===============================================
  Hits                    ?     2018           
  Misses                  ?    16704           
  Partials                ?        0           

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 f0fa255...28c2d73. Read the comment docs.

@vyasr
Copy link
Contributor Author

vyasr commented Aug 25, 2021

rerun tests

1 similar comment
@vyasr
Copy link
Contributor Author

vyasr commented Aug 27, 2021

rerun tests

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.

See comments. Overall this design is good! I have a few suggestions.

cpp/src/join/conditional_join.cu Outdated Show resolved Hide resolved
cpp/src/transform/compute_column.cu Outdated Show resolved Hide resolved
cpp/src/ast/expression_parser.cpp Outdated Show resolved Hide resolved
cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from bdice September 7, 2021 15:56
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.

Beautiful! I like the changes you made since my last review. It feels much cleaner.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 7, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a02f888 into rapidsai:branch-21.10 Sep 7, 2021
@vyasr vyasr deleted the fix/issue_8831 branch January 14, 2022 18:05
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 bug Something isn't working 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.

[BUG] AST produces non-null result when expression results in null scalar
4 participants