[Substrait] Implement first version of NamedTableOp. #797
Conversation
ingomueller-net
force-pushed
the
substrait-named-table
branch
from
6a14dca to
17e9971
Compare
lib/Target/SubstraitPB/Export.cpp
Outdated
| return failure(); | ||
|
|
||
| Region &body = op.getBodyRegion(); | ||
| if (llvm::range_size(body.getOps()) != 1) |
There was a problem hiding this comment.
This is a little wasteful/one could early exit without computing total size.
There was a problem hiding this comment.
How? The only way I can think of is something like ++body.getOps().begin() != body.op_end() but that's much less readable, isn't it?
There was a problem hiding this comment.
Indeed it is, one could also if the first is a terminator, I think your version from comment would be cheaper than that though. One could get creative with zip and seq too , but I think simplest may be a lambda with loop inside from readability point of view (I mean this may not matter, I meant "little" in the non-British use of little :-))
There was a problem hiding this comment.
Shame! std::ranges::take_view would be helpful here but it's C++20.
But, actually, maybe std::next(body.op_begin()) != body.op_end() isn't too bad?
There was a problem hiding this comment.
Yet another thought: if the IR is valid, then the range of the size is 1, so llvm::range_size acutally has acceptable cost. Do we really have to optimize for runtime on invalid IR?
There was a problem hiding this comment.
Based on that last argument, I propose to revert my latest change and go with the initial version. WDYT?
There was a problem hiding this comment.
Resolving this discussion because (1) the related line wasn't part of this commit, (2) it's the only one left for an otherwise approved PR, (3) we can always come back later: #815.
Thanks! In the "Files changed" tab, where you're probably doing the review, you can change "Show changes from all commits" to a single commit or even a range (by holding shift). |
ingomueller-net
force-pushed
the
substrait-named-table
branch
4 times, most recently
from
7657e6b to
4ad9d15
Compare
|
|
||
| include "mlir/IR/OpBase.td" | ||
|
|
||
| def Substrait_RelOpInterface : OpInterface<"RelOpInterface"> { |
There was a problem hiding this comment.
OOC are you going to keep a ordering here of some sorts? (E.g., top to bottom or sorted or by type or ...)
There was a problem hiding this comment.
I haven't made up my mind for this file yet. For SubstraitOps.td, I have sections that correspond to the sections in the spec and the protobuf files of Substrait, and then sort alphabetically inside each section. I guess I'll do the same here (once I figure out if we need sections and which they are).
ingomueller-net
force-pushed
the
substrait-named-table
branch
from
4ad9d15 to
ad82b52
Compare
ingomueller-net
force-pushed
the
substrait-named-table
branch
from
7f4b5fb to
8686209
Compare
ingomueller-net
force-pushed
the
substrait-named-table
branch
2 times, most recently
from
9b2974b to
8d4952f
Compare
This op corresponds to a `Rel` message with a `ReadRel` message that, in turn, has a `NamedTable` message. With this op, we are capable of representing a first complete plan in our dialect that we can serialize to protobuf and execute (by copy'n'pasting into an external consumer).
ingomueller-net
force-pushed
the
substrait-named-table
branch
from
8d4952f to
196ad1d
Compare
This reverts commit 3f0b68f.
…lvm-sandbox#797) This op corresponds to a `Rel` message with a `ReadRel` message that, in turn, has a `NamedTable` message. With this op, we are capable of representing a first complete plan in our dialect that we can serialize to protobuf and execute (by copy'n'pasting into an external consumer). Signed-off-by: Ingo Müller <[email protected]>
…lvm-sandbox#797) This op corresponds to a `Rel` message with a `ReadRel` message that, in turn, has a `NamedTable` message. With this op, we are capable of representing a first complete plan in our dialect that we can serialize to protobuf and execute (by copy'n'pasting into an external consumer). Signed-off-by: Ingo Müller <[email protected]>
This op corresponds to a
Relmessage with aReadRelmessage that, in turn, has aNamedTablemessage. With this op, we are capable of representing a first complete plan in our dialect that we can serialize to protobuf and execute (by copy'n'pasting into an external consumer).This PR is based on and, therefor, includes #795.