Copilot commented on code in PR #13368:
URL: https://github.com/apache/gluten/pull/13368#discussion_r4344482903
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1022,10 +1022,11 @@ JNIEXPORT jlong JNICALL
Java_org_apache_gluten_vectorized_HashJoinBuilder_native
cb.push_back(ObjectStore::retrieve<ColumnarBatch>(handle));
}
- if (numThreads == 1) {
- // Use default global pool for driver-side build
- // The hash table will be serialized and broadcast, so it doesn't need
runtime's pool
- // Using runtime pool causes lifecycle management issues
+ auto* executor = VeloxBackend::get()->broadcastHashTableBuildExecutor();
+ if (numThreads == 1 || executor == nullptr) {
Review Comment:
This condition is slightly inconsistent with the documented semantics (“<= 1
uses the serial build path”). Using `numThreads <= 1` instead of `numThreads ==
1` makes the behavior clearer and more robust for edge values (e.g., 0) without
relying on `executor == nullptr` as the only guard.
##########
cpp/velox/compute/VeloxBackend.cc:
##########
@@ -259,9 +259,17 @@ void VeloxBackend::init(
std::make_unique<folly::CPUThreadPoolExecutor>(ioThreads,
folly::CPUThreadPoolExecutor::makeLifoSemQueue());
}
- const auto broadcastHashTableBuildThreads =
- std::max<int32_t>(1,
backendConf_->get<int32_t>(kBroadcastHashTableBuildThreads,
numTaskSlotsPerExecutor));
- broadcastHashTableBuildExecutor_ =
std::make_unique<folly::CPUThreadPoolExecutor>(broadcastHashTableBuildThreads);
+ const int32_t broadcastHashTableBuildThreads = std::max<int32_t>(1, [&]() {
+ if (!backendConf_->valueExists(kBroadcastHashTableBuildThreads)) {
+ LOG(WARNING) << kBroadcastHashTableBuildThreads << " is not set. Falling
back to " << kNumTaskSlotsPerExecutor
+ << " (" << numTaskSlotsPerExecutor << ").";
+ return numTaskSlotsPerExecutor;
+ }
+ return backendConf_->get<int32_t>(kBroadcastHashTableBuildThreads).value();
+ }());
Review Comment:
The config being unset is a normal/expected case for common Spark paths
(especially if defaults are expected to be resolved elsewhere), so logging at
WARNING can create noisy logs and false alerts. Consider downgrading this to
INFO/VLOG, and/or using a rate-limited form (e.g., “log once” / `LOG_EVERY_N`)
so production logs aren’t flooded when many backends initialize.
##########
cpp/velox/config/VeloxConfig.h:
##########
@@ -170,10 +170,10 @@ const std::string kVeloxAsyncTimeoutOnTaskStopping =
"spark.gluten.sql.columnar.backend.velox.asyncTimeoutOnTaskStopping";
const int32_t kVeloxAsyncTimeoutOnTaskStoppingDefault = 30000; // 30s
-// By default, executors use the number of task slots per executor, with a
-// minimum of one thread. When driver-side broadcast hash-table build is
enabled
-// and Spark is not running in local mode, Scala-side logic in VeloxListenerApi
-// overrides the driver default to use spark.driver.cores instead.
+// VeloxListenerApi resolves Spark defaults per process: executor task slots,
+// driver cores, or local master cores. Native callers that omit this setting
+// fall back to the number of task slots per executor. Values less than or
equal to one use the serial build path
+// without creating a dedicated thread pool.
Review Comment:
The comment at line 175 reads as a run-on sentence and is harder to scan in
reviews/docs. Consider splitting it into two sentences (or wrapping lines) and
adding ending punctuation so it’s consistent with nearby documentation style.
--
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]
