Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3377948655
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -72,6 +72,15 @@ const std::string kHiveDefaultPartition =
"__HIVE_DEFAULT_PARTITION__";
} // namespace
+namespace {
+std::string getVeloxTaskId(const SparkTaskInfo& taskInfo) {
+ if (taskInfo.executionId != -1) {
+ return fmt::format("Gluten_Execution_{}",
std::to_string(taskInfo.executionId));
+ }
+ return fmt::format("Gluten_Execution_{}", "");
+}
Review Comment:
`getVeloxTaskId` currently returns the same string for every Spark task
within the same SQL execution (and even returns `Gluten_Execution_` with an
empty suffix when `executionId == -1`). Velox task IDs must be unique per Spark
task/partition; otherwise concurrent tasks can collide in Velox's global task
registry and lead to wrong results or runtime failures.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -135,9 +135,12 @@ case class BroadcastHashJoinExecTransformer(
override def columnarInputRDDs: Seq[RDD[ColumnarBatch]] = {
val streamedRDD = getColumnarInputRDDs(streamedPlan)
val executionId =
sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ var cacheKey = ""
if (executionId != null) {
+ cacheKey = "Gluten_Execution_" + executionId + ":" + buildHashTableId
GlutenDriverEndpoint.collectResources(executionId, buildBroadcastTableId)
Review Comment:
`cacheKey` is being passed as `BroadcastHashJoinContext.buildHashTableId`
and then down to JNI as the hash table cache key. However, the Substrait
JoinRel `hashtableid` (and join parameters) still use `buildHashTableId` (plan
id) without the `Gluten_Execution_<id>:` prefix, so the native join node won't
be able to find the injected table in `HashTableCache` and executor-side
cleanup via `GlutenDriverEndpoint` will also target a different id. Use a
single, consistent hash table id across build/inject, join plan, and cleanup.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -17,9 +17,9 @@
set -exu
CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd)
-VELOX_REPO=https://github.com/IBM/velox.git
-VELOX_BRANCH=dft-2026_06_05
-VELOX_ENHANCED_BRANCH=ibm-2026_06_05
+VELOX_REPO=https://github.com/JkSelf/velox.git
+VELOX_BRANCH=dft-2026_06_05-hashtable-cache
+VELOX_ENHANCED_BRANCH=ibm-2026_06_05-hashtable-cache
Review Comment:
Defaulting the Velox dependency checkout to a personal GitHub fork
(`JkSelf/velox`) is risky for CI reproducibility and supply-chain/security. The
default repo/branch should point to an org-controlled upstream (e.g., IBM or
facebookincubator), while still allowing developers to override via
`--velox_repo` / `--velox_branch` for local testing.
--
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]
