Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3393312304
##########
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` falls back to a constant "Gluten_Execution_" when
`executionId` is unavailable, which makes QueryCtx IDs non-unique across
tasks/queries and can cause HashTableCache or other per-query state to collide.
##########
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) {
- GlutenDriverEndpoint.collectResources(executionId, buildBroadcastTableId)
+ cacheKey = "Gluten_Execution_" + executionId + ":" + buildHashTableId
+ GlutenDriverEndpoint.collectResources(executionId, cacheKey)
Review Comment:
`cacheKey` is passed down as `BroadcastHashJoinContext.buildHashTableId`
(and then used as the native HashTableCache injection key), but the probe-side
Substrait JoinRel `hashtableid` is generated elsewhere from the build plan ID.
These two IDs need to be identical for Velox HashTableCache reuse to work;
also, when `executionId` is null the key becomes `Gluten_Execution_:` + id,
which can collide across different SQL executions.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -448,29 +448,11 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
} else if (
sJoin.has_advanced_extension() &&
SubstraitParser::configSetInOptimization(sJoin.advanced_extension(),
"isBHJ=")) {
- std::string hashTableId = sJoin.hashtableid();
-
- std::shared_ptr<core::OpaqueHashTable> opaqueSharedHashTable = nullptr;
- bool joinHasNullKeys = false;
-
- try {
- auto hashTableBuilder =
ObjectStore::retrieve<gluten::HashTableBuilder>(getJoin(hashTableId));
- joinHasNullKeys = hashTableBuilder->joinHasNullKeys();
- auto originalShared = hashTableBuilder->hashTable();
- opaqueSharedHashTable = std::shared_ptr<core::OpaqueHashTable>(
- originalShared,
reinterpret_cast<core::OpaqueHashTable*>(originalShared.get()));
-
- LOG(INFO) << "Successfully retrieved and aliased HashTable for reuse.
ID: " << hashTableId;
- } catch (const std::exception& e) {
- LOG(WARNING)
- << "Error retrieving HashTable from ObjectStore: " << e.what()
- << ". Falling back to building new table. To ensure correct results,
please verify that spark.gluten.velox.buildHashTableOncePerExecutor.enabled is
set to false.";
- opaqueSharedHashTable = nullptr;
- }
-
+ const auto& hashTableId = sJoin.hashtableid();
+ const auto joinNodeId = hashTableId.empty() ? nextPlanNodeId() :
hashTableId;
// Create HashJoinNode node
return std::make_shared<core::HashJoinNode>(
- nextPlanNodeId(),
+ joinNodeId,
Review Comment:
Velox HashJoinNode is now created with `joinNodeId = sJoin.hashtableid()`.
If `hashtableid` can be a plain numeric string (e.g., Spark plan ID), it can
collide with `nextPlanNodeId()` (sequential numeric strings starting at 0),
leading to duplicate PlanNodeIds within a plan and/or incorrect HashTableCache
lookups. Ensure `hashtableid` is globally unique within the converted plan
(e.g., prefixed key) and matches the build-side cache injection key.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -267,7 +278,7 @@ trait HashJoinLikeExecTransformer extends BaseJoinExec with
TransformSupport {
inputBuildOutput,
context,
operatorId,
- buildPlan.id.toString
+ canonicalBuildHashTableId(buildPlan)
Review Comment:
The JoinRel `hashTableId` is currently derived from
`canonicalBuildHashTableId(buildPlan)` instead of the `buildHashTableId`
produced by `genJoinParametersInternal()`. With this PR, Velox uses
`hashtableid` as the HashJoinNode ID, so this should match the actual cache key
used by the build side; otherwise probe-side lookup can miss the cached table
and IDs may collide with sequential `nextPlanNodeId()` values.
##########
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_06
-VELOX_ENHANCED_BRANCH=ibm-2026_06_06
+VELOX_REPO=https://github.com/JkSelf/velox.git
+VELOX_BRANCH=dft-2026_06_06-hashtable-cache
+VELOX_ENHANCED_BRANCH=ibm-2026_06_06-hashtable-cache
Review Comment:
The Velox bootstrap script now defaults to cloning from a personal fork
(JkSelf/velox). This is a supply-chain/reproducibility risk for CI and
contributors; the default should remain an approved upstream (e.g., IBM/velox)
with optional overrides via flags.
--
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]
