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

Migrated Dynamic AST Expression Trees in Benchmarks and Tests to use AST Tree #17697

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

lamarrr
Copy link
Contributor

@lamarrr lamarrr commented Jan 8, 2025

Description

Follows-up and closes #17400.
This merge cleans up some of the ast tree expressions in CUDF's benchmarks and tests, specifically the dynamically constructed ones.
The other existing AST trees/expressions in the tests and benchmarks are linear and without complicated lifetimes.

Checklist

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

Copy link

copy-pr-bot bot commented Jan 8, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jan 8, 2025
…ee and made ast tree use unique_ptr in place of shared_ptr
@lamarrr lamarrr changed the title Migrated Benchmarks and Tests to use AST Tree Migrated Nested Expressions in Benchmarks and Tests to use AST Tree Jan 14, 2025
@lamarrr lamarrr changed the title Migrated Nested Expressions in Benchmarks and Tests to use AST Tree Migrated Dynamic AST Expression Trees in Benchmarks and Tests to use AST Tree Jan 14, 2025
@lamarrr lamarrr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 14, 2025
@lamarrr lamarrr marked this pull request as ready for review January 14, 2025 17:39
@lamarrr lamarrr requested a review from a team as a code owner January 14, 2025 17:39
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

This is a nice cleanup, thanks!

cpp/include/cudf/ast/expressions.hpp Show resolved Hide resolved
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.

All looks fine to me. Thanks!

cpp/benchmarks/ast/transform.cpp Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Jan 24, 2025

@lamarrr any reason we shouldn't go ahead and merge this?

@lamarrr
Copy link
Contributor Author

lamarrr commented Jan 24, 2025

@lamarrr any reason we shouldn't go ahead and merge this?

I was waiting on the CI to pass, the pre-commit.ci task was failing and interrupting other CI tasks

@lamarrr
Copy link
Contributor Author

lamarrr commented Jan 24, 2025

/merge

@rapids-bot rapids-bot bot merged commit c57cb6e into rapidsai:branch-25.02 Jan 24, 2025
106 of 107 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[FEA] Migrate Unit tests to use AST Tree
4 participants