-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: extension spaces, functions, aggregation relations to binary #67
Conversation
EpsilonPrime
commented
Jun 7, 2023
•
edited
Loading
edited
- Features
- Extension spaces and functions are now emitted to the binary plan
- Details such as invocation, measures, groupings now implemented for aggregation relations and sorts for sort relations
- Fixes
- Type conversion to text standardized for schemas and extensions
- Changes lookupSymbolByLocation to return a pointer as the named lookup.
* Fields are now emitted in expressions completing expression handling. * Schemas and sources are also now implemented.
…til all of the features have caught up.
5b89eb1
to
b5c4de5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor thoughts, nothing too major.
@@ -2,10 +2,13 @@ | |||
|
|||
#include "substrait/textplan/converter/PlanPrinterVisitor.h" | |||
|
|||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some stringstream operations going on so it's currently marked as being included. Eventually we should run an IWYU on the project (which we need to set up -- I added issue #70 for that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use <sstream>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use <sstream>
@@ -79,8 +86,16 @@ ParseResult parseStream(antlr4::ANTLRInputStream stream) { | |||
*pipelineVisitor->getSymbolTable(), pipelineVisitor->getErrorListener()); | |||
try { | |||
relationVisitor->visitPlan(tree); | |||
} catch (std::invalid_argument ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment what you are trying to catch here? Do you want to just grab any std::exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment. When using std::exception it didn't catch the any_cast exception I was looking for.