Expose expression base class publicly and simplify public AST API #9045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…r to recompilation of AST-consuming code when AST internals change.
vyasr
added
2 - In Progress
10 tasks
|
@jlowe this PR addresses a number of the issues that we have discussed offline (some of which are captured in #8145). Java tests will not pass until I update those bindings (I'm happy to take a stab at it), but I'd like to reach consensus on the C++ changes first so that we can make sure we're on the same page with the API. |
vyasr
added
3 - Ready for Review
jlowe
reviewed
Aug 17, 2021
|
rerun tests |
|
@jlowe once this PR is approved would it be best to hold off merging until you've had a chance to update the sparks-rapids consumers of this API? |
jlowe
reviewed
Aug 18, 2021
java/src/test/java/ai/rapids/cudf/ast/CompiledExpressionTest.java
Outdated
Show resolved
Hide resolved
…add support for this in expression compilation.
jlowe
approved these changes
Aug 18, 2021
dillon-cullinan
approved these changes
Aug 18, 2021
|
@gpucibot merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR performs the renames
ast::expression->ast::operationandast::node->ast::expression, which more accurately reflects that literals and column references are also valid expressions. The newexpressionclass is publicly exposed, allowing client code to maintain a list of expression pointers composed of different subclasses of expression, and all APIs now accept arbitrary expressions as input so that it is possible to evaluate e.g. a raw column reference on a table. The oldnodes.hppandoperators.hppfiles have been merged into a singleexpressions.hppheader that now contains the entire public API for AST construction, but some implementation details have been moved into anexpressions.cppsource file so dependencies on detail APIs like theexpression_parserare restricted to a single TU, which dramatically simplifies recompilation of the code on small changes. Many more parameters are now accepted by (const) ref rather than by value, avoiding unnecessary copies and improving const correctness of the code.