westonpace commented on code in PR #12672: URL: https://github.com/apache/arrow/pull/12672#discussion_r860412364
########## cpp/src/arrow/engine/substrait/util.cc: ########## @@ -0,0 +1,146 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "arrow/engine/substrait/util.h" +#include "arrow/util/async_generator.h" +#include "arrow/util/async_util.h" + +namespace arrow { + +namespace engine { + +/// \brief A SinkNodeConsumer specialized to output ExecBatches via PushGenerator +class ARROW_ENGINE_EXPORT SubstraitSinkConsumer : public compute::SinkNodeConsumer { + public: + explicit SubstraitSinkConsumer( + AsyncGenerator<util::optional<compute::ExecBatch>>* generator) + : producer_(MakeProducer(generator)) {} + + Status Consume(compute::ExecBatch batch) override { + // Consume a batch of data + bool did_push = producer_.Push(batch); + if (!did_push) return Status::ExecutionError("Producer closed already"); + return Status::OK(); + } + + Status Init(const std::shared_ptr<Schema>& schema, + compute::BackpressureControl* backpressure_control) override { + return Status::OK(); + } + + static arrow::PushGenerator<util::optional<compute::ExecBatch>>::Producer MakeProducer( + AsyncGenerator<util::optional<compute::ExecBatch>>* out_gen); + + Future<> Finish() override { + producer_.Push(IterationEnd<util::optional<compute::ExecBatch>>()); + if (producer_.Close()) { + return Future<>::MakeFinished(); + } + return Future<>::MakeFinished( + Status::ExecutionError("Error occurred in closing the batch producer")); Review Comment: Specifically, it adds the attribute: `warn_unused_result`. Attributes are compiler-specific features that aren't technically part of the language. There are many attributes and they can have all kinds of purposes. By wrapping the attribute in a macro we can basically say "use this feature if the compiler supports it, otherwise do nothing" This specific attribute (`warn_unused_result`) is documented [here](https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html). It tells the compiler that the caller must do something with this result or it will emit a warning. We apply it very sparingly in Arrow but it is applied to `arrow::Status` and `arrow::Result`. That is why something like this: ``` thread_pool->Spawn(...); ``` will emit a warning: ``` warning: ignoring return value of ... declared with attribute 'warn_unused_result' [-Wunused-result] ``` It's a hint to help the developer make sure they are accounting for anything that might possibly fail. However, sometimes we know that we are invoking one of these functions in a way that could never ever fail and we want to ignore the result. Or even if something did fail we would just ignore the failure. In this case you have to use `ARROW_UNUSED` to avoid the warning: ``` // No warning will be generated ARROW_UNUSED(thread_pool->Spawn(...)); ``` Again, this should be used very sparingly, as it is much better to use `RETURN_NOT_OK` or `ARROW_ASSIGN_OR_RAISE`. So my original comment was to say that even though I suggested: ``` ARROW_UNUSED(producer_.Close()); ``` ...it would be perfectly fine to do... ``` producer_.Close(); ``` ...because I was suggesting `ARROW_UNUSED` purely for documentation/readability purposes and not because it would actually be required. -- 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]
