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

Added ast tree to simplify expression lifetime management #17156

Merged

rapids-bot merged 29 commits into rapidsai:branch-24.12 from lamarrr:ast-expr-enhancement

Nov 7, 2024

Contributor

lamarrr commented Oct 23, 2024

Description

This merge request follows up on #10744.
It attempts to simplify managing expressions by adding a class called an ast tree. The ast tree manages and holds related expressions together. When the tree is destroyed, all the expressions are also destroyed. Ideally we would use a bump allocator for allocating the expressions instead of std::vector<std::unique_ptr<expression>>.

We'd also ideally use a cuda::std::inplace_vector for storing the operands of the operation class, but that's in a newer version of CCCL.

Checklist

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


          added ast tree to simplify expression lifetime management

4d60344

lamarrr requested a review from a team as a code owner

October 23, 2024 19:25

lamarrr requested review from mythrocks and nvdbaranec

October 23, 2024 19:25

github-actions bot added the libcudf label

lamarrr added feature request non-breaking labels

lamarrr added 3 commits

October 23, 2024 20:28


          Merge branch 'branch-24.12' into ast-expr-enhancement

bf9d71f


          updated copyright notices

58c12ba


          Merge branch 'ast-expr-enhancement' of https://github.com/lamarrr/cudf …

479a7b0

…into ast-expr-enhancement

bdice reviewed

View reviewed changes

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved


          updated ast tree documentation

04414db

wence- reviewed

View reviewed changes

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/src/ast/expressions.cpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

lamarrr added 10 commits

October 28, 2024 13:49


          fixed ast tree docs formatting

7d398f1


          temporarily changed back to using vector for storing ast operation's …

6c82411

…operands


          made operation arity check throw std::invalid_argument

e3b4823


          Merge branch 'branch-24.12' into ast-expr-enhancement

5695f29


          corrected ast builder push return type

342b184


          updated documentation for ast tree member functions

daa882b


          started drafting ast tree test

239bfd4


          updated ast tree expression management

ea1d430


          added ast tree tests

4b7eebd


          updated ast transform test

e17c2ef

github-actions bot added the CMake label

lamarrr added 3 commits

October 29, 2024 15:46


          updated ast tree documentation

d9fa6d8


          Merge remote-tracking branch 'upstream/branch-24.12' into ast-expr-en…

e33a7a0

…hancement


          Merge remote-tracking branch 'upstream/branch-24.12' into ast-expr-en…

e356ee5

…hancement

vyasr reviewed

View reviewed changes

cpp/include/cudf/ast/detail/expression_parser.hpp Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Show resolved Hide resolved

vyasr approved these changes

View reviewed changes

lamarrr requested a review from bdice

November 1, 2024 15:02

lamarrr requested a review from wence-

November 1, 2024 15:02

wence- approved these changes

View reviewed changes

Contributor

wence- left a comment •

edited

Loading

Trivial comment suggestions (my approval has no power here)

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/ast/expressions.hpp Outdated Show resolved Hide resolved

cpp/tests/ast/ast_tree_tests.cpp Outdated Show resolved Hide resolved

lamarrr and others added 2 commits

November 4, 2024 18:09


          Update cpp/include/cudf/ast/expressions.hpp

8f2265c

Co-authored-by: Lawrence Mitchell <[email protected]>


          Update cpp/include/cudf/ast/expressions.hpp

8a6dac6

Co-authored-by: Lawrence Mitchell <[email protected]>

github-actions bot assigned lamarrr

lamarrr and others added 6 commits

November 4, 2024 18:10


          Update cpp/include/cudf/ast/expressions.hpp

26b2989

Co-authored-by: Lawrence Mitchell <[email protected]>


          Update cpp/include/cudf/ast/expressions.hpp

aa1d17b

Co-authored-by: Lawrence Mitchell <[email protected]>


          Update cpp/include/cudf/ast/expressions.hpp

4cd078f

Co-authored-by: Lawrence Mitchell <[email protected]>


          Update cpp/tests/ast/ast_tree_tests.cpp

279f047

Co-authored-by: Lawrence Mitchell <[email protected]>


          Merge branch 'branch-24.12' into ast-expr-enhancement

984e34f


          Merge branch 'branch-24.12' into ast-expr-enhancement

6afd563

bdice approved these changes

View reviewed changes

Contributor

bdice left a comment

Nice work!

karthikeyann reviewed

View reviewed changes

cpp/include/cudf/ast/expressions.hpp Show resolved Hide resolved


          Merge branch 'branch-24.12' into ast-expr-enhancement

1dc38fc

karthikeyann approved these changes

View reviewed changes

Contributor

karthikeyann left a comment

LGTM 👍

lamarrr and others added 2 commits

November 6, 2024 22:32


          Merge branch 'branch-24.12' into ast-expr-enhancement

72b2f58


          Merge branch 'branch-24.12' into ast-expr-enhancement

dccfeb9

github-actions bot assigned vyasr

Contributor

vyasr commented Nov 7, 2024

/merge

1 similar comment

Contributor Author

lamarrr commented Nov 7, 2024

/merge

rapids-bot bot merged commit 4cbc15a into rapidsai:branch-24.12

102 checks passed

lamarrr deleted the ast-expr-enhancement branch

November 7, 2024 14:05

bdice mentioned this pull request

[BUG] AST_TEST TransformTest.DeeplyNestedArithmeticLogicalExpression fails when run in a debug build #17274

Closed

vyasr mentioned this pull request

[FEA] Make cudf::ast::operation own its subexpressions #10744

Closed

lamarrr mentioned this pull request

[FEA] Migrate Unit tests to use AST Tree #17400

Open

bdice mentioned this pull request

Support reading bloom filters from Parquet files and filter row groups using them #17289

Open

3 tasks

mhaseeb123 mentioned this pull request

Simplify expression transformer in Parquet predicate pushdown with ast::tree #17587

Merged

3 tasks

rapids-bot bot pushed a commit that referenced this pull request


          Simplify expression transformer in Parquet predicate pushdown with `a…

2837a45

…st::tree` (#17587)

This PR simplifies the StatsAST expression transformer in Parquet reader's predicate pushdown using `ast::tree` from (#17156). 

This PR is a follow up to @bdice's comment at #17289 (comment). Similar changes for the `BloomfilterAST` expression converter have been incorporated in the PR #17289.

Related to #17164

Authors:
  - Muhammad Haseeb (https://github.com/mhaseeb123)

Approvers:
  - Karthikeyan (https://github.com/karthikeyann)
  - Vukasin Milovanovic (https://github.com/vuule)
  - Bradley Dice (https://github.com/bdice)

URL: #17587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bdice bdice approved these changes

karthikeyann karthikeyann approved these changes

vyasr vyasr approved these changes

wence- wence- approved these changes

mythrocks Awaiting requested review from mythrocks mythrocks is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

nvdbaranec Awaiting requested review from nvdbaranec nvdbaranec is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Labels

CMake feature request libcudf non-breaking