malinjawi commented on code in PR #12592:
URL: https://github.com/apache/gluten/pull/12592#discussion_r4347476492
##########
cpp/velox/operators/reader/FileReaderIterator.h:
##########
@@ -46,4 +53,14 @@ class FileReaderIterator : public ColumnarBatchIterator {
std::string path_;
};
+// Replays an empty input marker as an input iterator that yields no batches.
+class EmptyInputMarkerIterator final : public FileReaderIterator {
+ public:
+ explicit EmptyInputMarkerIterator(const std::string& path) :
FileReaderIterator(path) {}
+
+ std::shared_ptr<ColumnarBatch> next() override {
+ return nullptr;
+ }
Review Comment:
Done in 919d308ed, `EmptyInputMarkerIterator` is now a plain
`ColumnarBatchIterator`. The only reason it extended `FileReaderIterator` was
that `GenericBenchmark` cast every input to it for the read time metric. Those
casts are now `dynamic_cast`, and an empty input just contributes no read time.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -768,6 +768,16 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi with
Logging {
// IdentityBroadcastMode
Seq.empty
}
+ if (
+ mode.isInstanceOf[HashedRelationBroadcastMode] &&
+ VeloxConfig.get.benchmarkDumpRequested &&
+
VeloxConfig.get.getConf(VeloxConfig.VELOX_BROADCAST_BUILD_HASHTABLE_ONCE_PER_EXECUTOR)
+ ) {
+ logWarning(
+ s"${VeloxConfig.VELOX_BROADCAST_BUILD_HASHTABLE_ONCE_PER_EXECUTOR.key}
is ignored " +
+ "while whole stage dumping is requested: the broadcast hash table is
built per task " +
+ "so that the micro benchmark can replay the dumped build side.")
+ }
Review Comment:
Fair point, it now logs once per JVM (919d308ed).
##########
cpp/velox/operators/writer/VeloxColumnarBatchWriter.cc:
##########
@@ -57,7 +57,11 @@ arrow::Status VeloxColumnarBatchWriter::write(const
std::shared_ptr<ColumnarBatc
}
arrow::Status VeloxColumnarBatchWriter::close() {
- writer_->close();
+ // The writer takes its schema from the first batch, so it is only created
once something has been
+ // written. Closing a writer that never received a batch is a no-op rather
than a null dereference.
+ if (writer_) {
+ writer_->close();
+ }
return arrow::Status::OK();
Review Comment:
Velox's Writer::close() doesn't return a status, it throws on failure, so
errors already propagate. This is unchanged from before the PR.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
