feat: add aggregate op and extend functions accordingly
#70
Conversation
ingomueller-net
force-pushed
the
aggregate-op
branch
2 times, most recently
from
7734b7a to
1245335
Compare
ingomueller-net
changed the title
aggregation op and extend functions accordingly [WIP]aggrege op and extend functions accordingly
ingomueller-net
changed the title
aggrege op and extend functions accordinglyaggregate op and extend functions accordingly
ingomueller-net
force-pushed
the
aggregate-op
branch
6 times, most recently
from
e646898 to
7206f71
Compare
| AggregationInvocationAttr aggregationInvocation) { | ||
| if (aggregationInvocation && | ||
| aggregationInvocation.getValue() != AggregationInvocation::all) { | ||
| // The whitespace printed here compensates the trimming of whitespace in |
There was a problem hiding this comment.
Can't one add it there ? E.g., use " " or some such? (just OOC, not blocker)
There was a problem hiding this comment.
I haven't found a way that makes me perfectly happy: if I add it there, I get two whitespaces in case this custom directive doesn't print anything. So now the declarative format suppresses a whitespace before this directive, then (1) if the directive prints something, it first prints the whitespace, then that thing, (2) otherwise it doesn't print anything, and in both cases, the declarative format adds another whitespace.
| @@ -32,6 +33,30 @@ namespace pb = google::protobuf; | |||
|
|
|||
| namespace { | |||
|
|
|||
| // Copied from | |||
| // https://github.com/llvm/llvm-project/blob/dea33c/mlir/lib/Transforms/CSE.cpp. | |||
There was a problem hiding this comment.
Unfortunately, no, at least not currently. It's in the cpp file, so we'd have to move it upstream to some header to make it accessible.
lib/Target/SubstraitPB/Import.cpp
Outdated
| importAggregateFunction(ImplicitLocOpBuilder builder, | ||
| const AggregateFunction &message) { | ||
| MLIRContext *context = builder.getContext(); | ||
| Location loc = UnknownLoc::get(context); |
There was a problem hiding this comment.
Why not use the location from the ImplicitLocOpBuilder?
There was a problem hiding this comment.
Good idea! Fixed in latest commit.
| // If it's a new expression, assign new reference. | ||
| if (hasInserted) { | ||
| it->second = groupingExprOps.size() - 1; | ||
| groupingExprValues.emplace_back(exprOp.value()->getResult(0)); |
There was a problem hiding this comment.
And these will always have a single result?
There was a problem hiding this comment.
Yes, but currently only by convention. This is bothering for some time but I haven't gotten around to trying to improve things again. Ideally, I would like to attach the OpTrait::OneResult to the RelOpInterface and the ExpressionOpInterface but I didn't immediate manage to do that (and I am not sure if we can attach a trait to an interface). The best alternative I came up with is to add a getResult() method to the interface that simply does return getResult(0).
This PR adds support for the `AggregateRel` from the Substrait spec in the form of the `aggregate` op. This is arguably the most complex op implemented so far. It has an optional enum argument that requires custom parsing, several optional regions that require custom parsing, an attribute that depends on the presence and contents of the regions and requires custom parsing to omit it in the common case, and return types that depend on the two regions and the attribute. What's more, the current version of the spec is such that it is almost impossibly to interpret "grouping sets" because it relies on protobuf message equality, which is something can protobuf does not offer. The current implementation, thus, implements a best effort by using op equality instead (but needs to run CSE during export to ensure op uniqueness). Finally, the PR also extends the `call` op to represent also `AggregateFunction` messages (in addition to `ScalarFunction` messages), which are used by the new `aggregate` op.
ingomueller-net
force-pushed
the
aggregate-op
branch
from
7206f71 to
b31b4e4
Compare
This commit addresses a comment by @jpienaar by using the `Location` from the `ImplicitOpLocBuilder` instead of an `UnknownLoc` wherever one is available (including places there weren't previously affected by this PR). Signed-off-by: Ingo Müller <[email protected]>
ingomueller-net
force-pushed
the
aggregate-op
branch
from
f64114e to
ec0c556
Compare
This PR depends on and, therefor, includes #68.This PR adds support for the
AggregateRelfrom the Substrait spec in the form of theaggregateop. This is arguably the most complex op implemented so far. It has an optional enum argument that requires custom parsing, several optional regions that require custom parsing, an attribute that depends on the presence and contents of the regions and requires custom parsing to omit it in the common case, and return types that depend on the two regions and the attribute. What's more, the current version of the spec is such that it is almost impossibly to interpret "grouping sets" because it relies on protobuf message equality, which is something can protobuf does not offer. The current implementation, thus, implements a best effort by using op equality instead (but needs to run CSE during export to ensure op uniqueness). Finally, the PR also extends thecallop to represent alsoAggregateFunctionmessages (in addition toScalarFunctionmessages), which are used by the newaggregateop.