Copilot commented on code in PR #13115:
URL: https://github.com/apache/gluten/pull/13115#discussion_r4227189359
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxListenerApi.scala:
##########
@@ -317,6 +317,15 @@ object VeloxListenerApi {
parsed += (COLUMNAR_VELOX_CACHE_ENABLED.key -> "false")
}
+ if
(!parsed.contains(COLUMNAR_VELOX_BROADCAST_HASH_TABLE_BUILD_THREADS.key)) {
+ val defaultThreads = if (isDriver) {
+ Math.max(SparkResourceUtil.getDriverCores(conf), 1)
+ } else {
+ Math.max(SparkResourceUtil.getTaskSlots(conf), 1)
+ }
Review Comment:
On the driver this default is based on `spark.driver.cores`, whereas the
documented default and the previous IO-pool default are
`spark.gluten.numTaskSlotsPerExecutor` (derived from executor cores and
`spark.task.cpus`). In cluster deployments these can differ, so the claimed
default behavior is not preserved for driver-side builds; use
`getTaskSlots(conf)` for both processes.
##########
cpp/velox/compute/VeloxBackend.cc:
##########
@@ -254,6 +255,10 @@ 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, 1));
+ broadcastHashTableBuildExecutor_ =
std::make_unique<folly::CPUThreadPoolExecutor>(broadcastHashTableBuildThreads);
Review Comment:
The pool is still constructed unconditionally during backend initialization,
so every Velox backend allocates this broadcast-build executor even when no
parallel broadcast join is ever used. That contradicts the stated lazy-creation
design and the class guidance not to do heavy work at startup; create it on
first accessor (with one-time initialization) and keep the teardown reset.
This issue also appears on line 258 of the same file.
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -246,6 +246,15 @@ object VeloxConfig extends ConfigRegistry {
.intConf
.createOptional
+ val COLUMNAR_VELOX_BROADCAST_HASH_TABLE_BUILD_THREADS =
+
buildStaticConf("spark.gluten.sql.columnar.backend.velox.broadcastHashTableBuildThreads")
Review Comment:
The implementation registers
`spark.gluten.sql.columnar.backend.velox.broadcastHashTableBuildThreads`, while
the PR description names
`spark.gluten.sql.columnar.backend.velox.hashTableBuildThreads`. A user setting
the key described in the PR will be silently ignored by the native code; make
the key consistent across the description, Scala registry, native constant, and
documentation.
--
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]
