Copilot commented on code in PR #13115: URL: https://github.com/apache/gluten/pull/13115#discussion_r4245182688
########## cpp/velox/tests/VeloxBackendTest.cc: ########## @@ -0,0 +1,50 @@ +/* + * 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 <folly/executors/CPUThreadPoolExecutor.h> +#include <folly/futures/Future.h> +#include <gtest/gtest.h> +#include <thread> + +#include "compute/VeloxBackend.h" +#include "config/VeloxConfig.h" + +namespace gluten { + +class VeloxBackendTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + VeloxBackend::create(AllocationListener::noop(), {{kNumTaskSlotsPerExecutor, "0"}}); Review Comment: The test only sets `kNumTaskSlotsPerExecutor=0` but then asserts `ioExecutor()` is `nullptr`. That expectation isn’t implied by the setup and can break if the default IO thread count is non-zero. Either remove the `ioExecutor()` assertion (since it’s unrelated to the test name/purpose), or explicitly pass the IO-threads config set to 0 when creating the backend so the assertion matches the configured behavior. ########## cpp/velox/tests/VeloxBackendTest.cc: ########## @@ -0,0 +1,50 @@ +/* + * 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 <folly/executors/CPUThreadPoolExecutor.h> +#include <folly/futures/Future.h> +#include <gtest/gtest.h> +#include <thread> + +#include "compute/VeloxBackend.h" +#include "config/VeloxConfig.h" + +namespace gluten { + +class VeloxBackendTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + VeloxBackend::create(AllocationListener::noop(), {{kNumTaskSlotsPerExecutor, "0"}}); + } + + static void TearDownTestSuite() { + VeloxBackend::get()->tearDown(); + } +}; + +TEST_F(VeloxBackendTest, broadcastHashTableBuildExecutorWithZeroTaskSlots) { + auto* backend = VeloxBackend::get(); + ASSERT_EQ(backend->ioExecutor(), nullptr); Review Comment: The test only sets `kNumTaskSlotsPerExecutor=0` but then asserts `ioExecutor()` is `nullptr`. That expectation isn’t implied by the setup and can break if the default IO thread count is non-zero. Either remove the `ioExecutor()` assertion (since it’s unrelated to the test name/purpose), or explicitly pass the IO-threads config set to 0 when creating the backend so the assertion matches the configured behavior. ########## gluten-ut/spark35/src/test/backends-velox/org/apache/spark/sql/execution/joins/GlutenParallelBroadcastJoinSuite.scala: ########## @@ -0,0 +1,64 @@ +/* + * 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. + */ +package org.apache.spark.sql.execution.joins + +import org.apache.gluten.execution.BroadcastHashJoinExecTransformer + +import org.apache.spark.SparkConf +import org.apache.spark.sql.GlutenSQLTestsTrait +import org.apache.spark.sql.execution.ColumnarBroadcastExchangeExec + +class GlutenParallelBroadcastJoinSuite extends GlutenSQLTestsTrait { + + override def sparkConf: SparkConf = super.sparkConf + .set("spark.sql.adaptive.enabled", "false") + .set("spark.gluten.sql.columnar.backend.velox.IOThreads", "0") + .set("spark.gluten.velox.broadcast.build.targetBytesPerThread", "8KB") Review Comment: This test asserts `buildThreads > 1` but does not pin the broadcast hash-table build thread pool size. On single-core CI agents (or any environment where the computed default ends up as 1), this will fail even if the code is correct. To make the test deterministic, explicitly set `spark.gluten.sql.columnar.backend.velox.broadcastHashTableBuildThreads` to a value > 1 in the test configuration (and assert against that), or gate the assertion with an assumption based on available processors. ########## cpp/velox/config/VeloxConfig.h: ########## @@ -169,6 +169,11 @@ const std::string kVeloxAsyncTimeoutOnTaskStopping = "spark.gluten.sql.columnar.backend.velox.asyncTimeoutOnTaskStopping"; const int32_t kVeloxAsyncTimeoutOnTaskStoppingDefault = 30000; // 30s +// Size of the dedicated CPU thread pool for the parallel broadcast hash-table build. +// Defaults to the number of task slots per executor, with a minimum of one thread. Review Comment: The documented default here is incomplete relative to the behavior introduced in Scala (`VeloxListenerApi`), where driver-side default sizing is overridden to use `spark.driver.cores` (when driver-side broadcast hash-table build is enabled and not in local mode). Please update this comment to reflect the effective default on both executors and the driver to avoid misleading future maintainers. ########## gluten-ut/spark35/src/test/backends-velox/org/apache/spark/sql/execution/joins/GlutenParallelBroadcastJoinSuite.scala: ########## @@ -0,0 +1,64 @@ +/* + * 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. + */ +package org.apache.spark.sql.execution.joins + +import org.apache.gluten.execution.BroadcastHashJoinExecTransformer + +import org.apache.spark.SparkConf +import org.apache.spark.sql.GlutenSQLTestsTrait +import org.apache.spark.sql.execution.ColumnarBroadcastExchangeExec + +class GlutenParallelBroadcastJoinSuite extends GlutenSQLTestsTrait { + + override def sparkConf: SparkConf = super.sparkConf + .set("spark.sql.adaptive.enabled", "false") + .set("spark.gluten.sql.columnar.backend.velox.IOThreads", "0") + .set("spark.gluten.velox.broadcast.build.targetBytesPerThread", "8KB") + + for (buildOnDriver <- Seq(false, true)) { + testGluten(s"parallel broadcast hash table build with IO disabled, driver=$buildOnDriver") { + withSQLConf( + "spark.gluten.sql.columnar.backend.velox.driverSideBroadcastHashTableBuild" -> + buildOnDriver.toString, + "spark.gluten.velox.minTableRowsForParallelJoinBuild" -> "1" + ) { + withTable("parallel_broadcast_build") { + spark.range(4096) + .selectExpr("id", "sha2(cast(id as string), 256) AS value") + .write.saveAsTable("parallel_broadcast_build") + + val result = spark.sql(""" + |SELECT /*+ BROADCAST(b) */ p.id, b.value + |FROM parallel_broadcast_build p JOIN parallel_broadcast_build b + |ON p.id = b.id + |""".stripMargin) + val expected = spark.table("parallel_broadcast_build").collect().toSeq + checkAnswer(result, expected) + + val plan = result.queryExecution.executedPlan + assert(plan.collect { case j: BroadcastHashJoinExecTransformer => j }.nonEmpty) + val exchanges = plan.collect { case e: ColumnarBroadcastExchangeExec => e } + assert(exchanges.size == 1) + assert(exchanges.head.metrics("buildThreads").value > 1) Review Comment: This test asserts `buildThreads > 1` but does not pin the broadcast hash-table build thread pool size. On single-core CI agents (or any environment where the computed default ends up as 1), this will fail even if the code is correct. To make the test deterministic, explicitly set `spark.gluten.sql.columnar.backend.velox.broadcastHashTableBuildThreads` to a value > 1 in the test configuration (and assert against that), or gate the assertion with an assumption based on available processors. -- 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]
