Copilot commented on code in PR #12592:
URL: https://github.com/apache/gluten/pull/12592#discussion_r4347133664
##########
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:
`EmptyInputMarkerIterator` inherits from `FileReaderIterator` but doesn’t
actually perform file reading; depending on `FileReaderIterator`’s base
behavior (e.g., `hasNext()` semantics or constructor-side initialization), this
may introduce unnecessary coupling or unintended side effects. Prefer
implementing this as a minimal `ColumnarBatchIterator` that explicitly returns
`false` for `hasNext()` (if applicable) and `nullptr` for `next()`, or override
the relevant methods so the behavior is unambiguously “always empty” without
relying on `FileReaderIterator` internals.
##########
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:
`close()` always returns `arrow::Status::OK()`, even if `writer_->close()`
fails. Since callers use `GLUTEN_THROW_NOT_OK(writer->close())`, a close
failure would currently be silently ignored. Return the status from
`writer_->close()` (when `writer_` is present) so failures propagate correctly.
##########
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:
The warning condition will also be true when the “build hash table once per
executor” setting is just the default (i.e., not explicitly set by the user),
which may spam logs for every hashed broadcast join during dumping. Consider
emitting this warning only when the conf key was explicitly set (e.g., by
checking whether the key is present in SQLConf) and/or rate-limiting/log-once
per SparkSession.
--
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]
