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

Move AST evaluator into a separate header #8930

Merged
merged 4 commits into from
Aug 4, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Aug 3, 2021

This PR separates the logic for evaluating an AST from the logic for column computation using ASTs. This change ensures that conditional join code paths (and any other features that make use of libcudf AST evaluation in the future) do not depend on headers for the compute_column API. In addition to the improved conceptual clarity and separation of concerns, this significantly improves recompilation times when developing on either one of these features separately.

Contributes to #8145.

@vyasr vyasr added 3 - Ready for Review Ready for review by team code quality libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 3, 2021
@vyasr vyasr added this to the Conditional Joins milestone Aug 3, 2021
@vyasr vyasr self-assigned this Aug 3, 2021
@vyasr vyasr requested a review from a team as a code owner August 3, 2021 00:57
@vyasr vyasr requested review from ttnghia and jrhemstad August 3, 2021 00:57
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #8930 (dedc77d) into branch-21.10 (18f7c01) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head dedc77d differs from pull request most recent head 38690d9. Consider uploading reports for the commit 38690d9 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.10    #8930      +/-   ##
================================================
- Coverage         10.67%   10.61%   -0.07%     
================================================
  Files               110      116       +6     
  Lines             18271    19001     +730     
================================================
+ Hits               1951     2017      +66     
- Misses            16320    16984     +664     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/lists.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/methods.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/numerical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/struct.py 0.00% <ø> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <ø> (ø)
... and 74 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 c5a575c...38690d9. Read the comment docs.

Co-authored-by: Nghia Truong <[email protected]>
@vyasr
Copy link
Contributor Author

vyasr commented Aug 4, 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 improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants