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.
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: add support for emit in aggregate relations #122
feat: add support for emit in aggregate relations #122
Changes from 10 commits
fb5519e
d3dea20
c56d59b
f399361
f2f4245
054b2ed
522e999
ed0731c
517c705
181332d
737dadf
0c4ea54
51abce4
6b64b76
bd114dc
ce112d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Much nicer, thanks. It might be worthwhile to include the type of currentScope (and therefore the range of fieldReferences which would have been valid) below in the out of range error message
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.
I do agree that better error messages are useful but I believe the substrait validator is better suited for helping developers figure what is wrong with their plan generation (allowing this code to be simpler). I've added the type here though.
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.
What sort of more explicit check were you thinking?
I'm not familiar enough with substrait to know at what level aggregate nodes are the only relations with output field references. Do you mean they are the only ones supported by substrait-cpp? The only relation for which textplan will ever provide a representation? I don't think they're the only relations which substrait itself allows to have an output mapping...
I think the comment should explain this in greater depth. If it's the last thing (IE, part of general substrait knowledge) maybe a link to an appropriate section of the substrait doc is in order.
In any case, if this block of code is specific to
relationData->relation.rel_type_case() == ::substrait::proto::Rel::kAggregate
, it should probably be moved into the switch aboveThere 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.
This was relation into the comment below. I've replaced the output field references with a check on the incoming proto's type. I did not move it above as the function is currently separated into a section where the fields are populated with a later emit handling step. I can see some potential refactoring where those two parts of this rather long function could be split up.
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.
Why are there two implementations of this function (PlanPrinterVisitor.cpp:74)? The definitions are even different. If the difference is intentional, that's definitely worth a comment or at least renaming the functions to clarify their distinct behaviors. If the difference is not intentional, could this become
SymbolInfo::isAggregate
?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.
Long term there shouldn't be this function at all and just a single type in the representation. Right now the intermediate form can have both types (depending on how it was parsed) which needs to be addressed.
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.
In that case, a comment would definitely be advisable just to give future readers a clue that this is a stopgap and that in the future there should only be
RelationType::kAggregate
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.
This looks very similar to the code at the end of
InitialPlanProtoVisitor::updateLocalSchema
. Any chance it could be extracted to a helper function?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.
ditto
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.
more ditto