westonpace commented on code in PR #13914:
URL: https://github.com/apache/arrow/pull/13914#discussion_r957611273
##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -199,12 +217,22 @@ Result<DeclarationInfo> FromProto(const substrait::Rel&
rel, const ExtensionSet&
std::move(filesystem),
std::move(files),
std::move(format), {}));
- ARROW_ASSIGN_OR_RAISE(auto ds,
ds_factory->Finish(std::move(base_schema)));
+ ARROW_ASSIGN_OR_RAISE(auto ds, ds_factory->Finish(base_schema));
- return DeclarationInfo{
- compute::Declaration{
- "scan", dataset::ScanNodeOptions{std::move(ds),
std::move(scan_options)}},
- num_columns};
+ if (HasEmit(read)) {
+ ARROW_ASSIGN_OR_RAISE(auto emit_expressions, GetEmitInfo(read,
base_schema));
+ return DeclarationInfo{
+ compute::Declaration::Sequence(
+ {{"scan",
+ dataset::ScanNodeOptions{std::move(ds),
std::move(scan_options)}},
+ {"project",
compute::ProjectNodeOptions{std::move(emit_expressions)}}}),
+ static_cast<int>(emit_expressions.size()), std::move(base_schema)};
+ } else {
Review Comment:
@jvanstraten brought up a good point to me offline which is that we should
only do this else clause if things are direct. Also, I think `has_emit` is
only available in newer protoc versions. Can we change all these if/else
blocks to be...
```
switch (rel.emit_kind_case()) {
case substrait::RelCommon::EmitKindCase::kDirect:
// return no-emit case
case substrait::RelCommon::EmitKindCase::kEmit:
// return emit-case
default:
// return invalid status
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
