You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "JkSelf (via GitHub)" <gi...@apache.org> on 2026/05/28 06:12:47 UTC
[PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
JkSelf opened a new pull request, #12163:
URL: https://github.com/apache/gluten/pull/12163
<!--
Thank you for submitting a pull request! Here are some tips:
1. For first-time contributors, please read our contributing guide:
https://github.com/apache/gluten/blob/main/CONTRIBUTING.md
2. If necessary, create a GitHub issue for discussion beforehand to avoid duplicate work.
3. If the PR is specific to a single backend, include [VL] or [CH] in the PR title to indicate the
Velox or ClickHouse backend, respectively.
4. If the PR is not ready for review, please mark it as a draft.
-->
## What changes are proposed in this pull request?
<!--
Provide a clear and concise description of the changes introduced in this PR.
Ensure the PR description aligns with the code changes, especially after updates.
If applicable, include "Fixes #<GitHub_Issue_ID>" to automatically close the corresponding issue
when the PR is merged.
-->
## How was this patch tested?
<!--
Describe how the changes were tested, if applicable.
Include new tests to validate the functionality, if necessary.
For UI-related changes, attach screenshots to demonstrate the updates.
-->
## Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4873543483
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3439975339
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -105,6 +107,15 @@ trait HashJoinLikeExecTransformer extends BaseJoinExec with TransformSupport {
(right, left)
}
+ protected def canonicalBuildHashTableId(plan: SparkPlan): String = plan match {
+ case b: BroadcastQueryStageExec =>
+ canonicalBuildHashTableId(b.plan)
+ case r: ReusedExchangeExec =>
+ canonicalBuildHashTableId(r.child)
+ case other =>
+ other.id.toString
+ }
Review Comment:
`canonicalBuildHashTableId` currently returns the raw SparkPlan numeric `id` as the hash-table/cache key. On the native side, this ID is also used as a Velox `PlanNodeId` (see BHJ path in `SubstraitToVeloxPlan.cc`), while `nextPlanNodeId()` generates numeric string IDs as well. Using plain numbers makes collisions very likely, which can corrupt split routing and/or plan metrics. Prefix the canonical ID with a stable string (similar to the old `BuiltHashTable-` prefix) to keep it distinct from auto-generated node IDs.
##########
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_12
-VELOX_ENHANCED_BRANCH=ibm-2026_06_12
+VELOX_REPO=https://github.com/JkSelf/velox.git
+VELOX_BRANCH=dft-2026_06_12-hashtable-cache
+VELOX_ENHANCED_BRANCH=ibm-2026_06_12-hashtable-cache
Review Comment:
Defaulting the Velox dependency to a personal fork (`JkSelf/velox`) makes builds non-reproducible and introduces supply-chain / operational risk for anyone building Gluten without custom flags. Please keep the default pinned to an official / org-owned repository (e.g., `IBM/velox` or `facebookincubator/velox`) and apply the HashTableCache changes via a pinned commit/branch in that repo (or via `UPSTREAM_VELOX_PR_ID`/patching), while still allowing personal forks only through `--velox_repo` 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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4888599979
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4689359022
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4629690683
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3518085612
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,26 +458,8 @@ 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(),
Review Comment:
`joinNodeId` is computed but never used; when `sJoin.hashtableid()` is empty this still calls `nextPlanNodeId()` once (advancing the internal counter) and then calls it again for the node ID. This both introduces an unused-variable warning (often treated as error) and changes plan node ID sequencing, and it also leaves the cache key argument as an empty string in the empty-ID case.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -394,7 +405,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
+ lazy val buildHashTableId: String = canonicalBuildHashTableId(buildPlan)
override def genJoinParametersInternal(): (Int, Int, String) = {
(1, if (isNullAwareAntiJoin) 1 else 0, buildHashTableId)
Review Comment:
`buildHashTableId` is now canonicalized across `BroadcastQueryStageExec` / `ReusedExchangeExec`, which increases hash-table reuse across multiple BHJ operators. However, the cache key still only includes the (canonical) build plan ID, while the actual hash table build behavior changes with join semantics (e.g., semi/anti joins can drop duplicates when there is no filter). Reusing the same cache key across different join types / filter presence can cause the wrong hash table variant to be reused and lead to incorrect results.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -394,7 +405,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
+ lazy val buildHashTableId: String = canonicalBuildHashTableId(buildPlan)
Review Comment:
Typo in comment: "builded" should be "built".
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3361501059
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -111,11 +124,7 @@ WholeStageResultIterator::WholeStageResultIterator(
velox::core::PlanFragment planFragment{planNode, velox::core::ExecutionStrategy::kUngrouped, 1, emptySet};
std::shared_ptr<velox::core::QueryCtx> queryCtx = createNewVeloxQueryCtx();
task_ = velox::exec::Task::create(
- fmt::format(
- "Gluten_Stage_{}_TID_{}_VTID_{}",
- std::to_string(taskInfo_.stageId),
- std::to_string(taskInfo_.taskId),
- std::to_string(taskInfo.vId)),
+ getVeloxTaskId(taskInfo_),
std::move(planFragment),
0,
Review Comment:
When Spark SQL execution id is available, getVeloxTaskId() returns the same string for all tasks of the same query (e.g., "Gluten_Execution_123"). Passing that directly to velox::exec::Task::create can cause duplicate task IDs within an executor, which can break Velox task management/metrics and potentially return an existing task instead of creating a new one. Make the Velox task id unique per task (e.g., append vId/stage/taskAttempt) while keeping executionId as a prefix for grouping/debugging.
##########
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_04
-VELOX_ENHANCED_BRANCH=ibm-2026_06_04
+VELOX_REPO=https://github.com/JkSelf/velox.git
+VELOX_BRANCH=dft-2026_06_04-hashtable-cache
+VELOX_ENHANCED_BRANCH=ibm-2026_06_04-hashtable-cache
Review Comment:
The build script now defaults to cloning Velox from a personal fork (JkSelf/velox). This reduces build reproducibility and introduces supply-chain risk for anyone running the default build path. Defaults should point to an agreed upstream (e.g., IBM/velox or facebookincubator/velox) and personal forks should only be used via explicit --velox_repo/--velox_branch overrides or via UPSTREAM_VELOX_PR_ID patching.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3440054366
##########
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_12
-VELOX_ENHANCED_BRANCH=ibm-2026_06_12
+VELOX_REPO=https://github.com/JkSelf/velox.git
+VELOX_BRANCH=dft-2026_06_12-hashtable-cache
+VELOX_ENHANCED_BRANCH=ibm-2026_06_12-hashtable-cache
Review Comment:
`get-velox.sh` now defaults to cloning Velox from a personal fork (`JkSelf/velox`). This makes builds non-reproducible and introduces an avoidable supply-chain risk for anyone running the build scripts.
Please keep the default pinned to an official upstream (e.g., the previous `IBM/velox` default) and use `--velox_repo/--velox_branch` or `UPSTREAM_VELOX_PR_ID` when testing forks/PR branches.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4667408769
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4911644849
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4888836865
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4669765071
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3400573851
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/GlutenQueryContext.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.gluten.execution
+
+import org.apache.spark.SparkContext
+import org.apache.spark.sql.execution.SQLExecution
+
+import java.util.UUID
+
+object GlutenQueryContext {
+ val QueryIdLocalProperty: String = "gluten.query.id"
+ private val QueryExecutionIdLocalProperty: String = "gluten.query.execution.id"
+
+ private def sanitize(value: String): String = value.replaceAll("[^A-Za-z0-9_\\-]", "_")
+
+ /**
+ * Initializes a query-scoped id on the driver before any query jobs are submitted.
+ *
+ * The id is cached in Spark local properties so all later driver-side async paths can inherit the
+ * same value. A companion execution id property prevents reusing a stale query id after the
+ * driver thread switches to a different SQL execution.
+ */
+ def getOrCreateQueryId(sparkContext: SparkContext): String = synchronized {
+ val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ val existingQueryId = sparkContext.getLocalProperty(QueryIdLocalProperty)
+ val existingExecutionId = sparkContext.getLocalProperty(QueryExecutionIdLocalProperty)
+ if (existingQueryId != null && existingExecutionId == executionId) {
+ return existingQueryId
+ }
Review Comment:
When `executionId` is `null`, `existingExecutionId == executionId` can still be true (both null), causing a previously generated random query id to be reused across unrelated (non-SQLExecution) runs. That can lead to cache-key collisions and incorrect cross-query reuse. Fix by only reusing the cached query id when `executionId != null`, or by storing a non-null sentinel/token in `QueryExecutionIdLocalProperty` when `executionId` is missing so successive calls don’t match on null.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/GlutenQueryContext.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.gluten.execution
+
+import org.apache.spark.SparkContext
+import org.apache.spark.sql.execution.SQLExecution
+
+import java.util.UUID
+
+object GlutenQueryContext {
+ val QueryIdLocalProperty: String = "gluten.query.id"
+ private val QueryExecutionIdLocalProperty: String = "gluten.query.execution.id"
+
+ private def sanitize(value: String): String = value.replaceAll("[^A-Za-z0-9_\\-]", "_")
+
+ /**
+ * Initializes a query-scoped id on the driver before any query jobs are submitted.
+ *
+ * The id is cached in Spark local properties so all later driver-side async paths can inherit the
+ * same value. A companion execution id property prevents reusing a stale query id after the
+ * driver thread switches to a different SQL execution.
+ */
+ def getOrCreateQueryId(sparkContext: SparkContext): String = synchronized {
+ val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ val existingQueryId = sparkContext.getLocalProperty(QueryIdLocalProperty)
+ val existingExecutionId = sparkContext.getLocalProperty(QueryExecutionIdLocalProperty)
+ if (existingQueryId != null && existingExecutionId == executionId) {
+ return existingQueryId
+ }
+
+ val queryId = if (executionId != null) {
+ s"Gluten_Query_${sanitize(sparkContext.applicationId)}_Execution_$executionId"
+ } else {
+ val randomId = sanitize(UUID.randomUUID().toString)
+ s"Gluten_Query_${sanitize(sparkContext.applicationId)}_$randomId"
+ }
+
+ setQueryId(sparkContext, queryId, executionId)
Review Comment:
When `executionId` is `null`, `existingExecutionId == executionId` can still be true (both null), causing a previously generated random query id to be reused across unrelated (non-SQLExecution) runs. That can lead to cache-key collisions and incorrect cross-query reuse. Fix by only reusing the cached query id when `executionId != null`, or by storing a non-null sentinel/token in `QueryExecutionIdLocalProperty` when `executionId` is missing so successive calls don’t match on null.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/GlutenQueryContext.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.gluten.execution
+
+import org.apache.spark.SparkContext
+import org.apache.spark.sql.execution.SQLExecution
+
+import java.util.UUID
+
+object GlutenQueryContext {
+ val QueryIdLocalProperty: String = "gluten.query.id"
+ private val QueryExecutionIdLocalProperty: String = "gluten.query.execution.id"
+
+ private def sanitize(value: String): String = value.replaceAll("[^A-Za-z0-9_\\-]", "_")
+
+ /**
+ * Initializes a query-scoped id on the driver before any query jobs are submitted.
+ *
+ * The id is cached in Spark local properties so all later driver-side async paths can inherit the
+ * same value. A companion execution id property prevents reusing a stale query id after the
+ * driver thread switches to a different SQL execution.
+ */
+ def getOrCreateQueryId(sparkContext: SparkContext): String = synchronized {
+ val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ val existingQueryId = sparkContext.getLocalProperty(QueryIdLocalProperty)
+ val existingExecutionId = sparkContext.getLocalProperty(QueryExecutionIdLocalProperty)
+ if (existingQueryId != null && existingExecutionId == executionId) {
+ return existingQueryId
+ }
+
+ val queryId = if (executionId != null) {
+ s"Gluten_Query_${sanitize(sparkContext.applicationId)}_Execution_$executionId"
+ } else {
+ val randomId = sanitize(UUID.randomUUID().toString)
+ s"Gluten_Query_${sanitize(sparkContext.applicationId)}_$randomId"
+ }
+
+ setQueryId(sparkContext, queryId, executionId)
+ queryId
+ }
+
+ def getQueryId(sparkContext: SparkContext): Option[String] =
+ Option(sparkContext.getLocalProperty(QueryIdLocalProperty))
+
+ def setQueryId(sparkContext: SparkContext, queryId: String, executionId: String): Unit = {
+ sparkContext.setLocalProperty(QueryIdLocalProperty, queryId)
+ sparkContext.setLocalProperty(QueryExecutionIdLocalProperty, executionId)
+ }
Review Comment:
`executionId` is modeled as a non-null `String`, but call sites pass the value from `sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)` which can be null. Even though Spark’s API accepts null values, the current signature makes null usage implicit and easy to misuse. Consider changing `setQueryId/withQueryId` to accept `Option[String]` (or provide an overload) and explicitly handle the unset case (e.g., clear the property or store a sentinel).
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
}
hashTableBuilders[0]->setHashTable(std::move(mainTable));
+
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(
+ hashTableId,
+ hashTableBuilders[0]->hashTable(),
+ hashTableBuilders[0]->joinHasNullKeys(),
+ defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableBuilders[0]);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_cloneHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(cacheKeyStr)) {
+ cache->injectTable(
+ cacheKeyStr, hashTableHandler->hashTable(), hashTableHandler->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableHandler);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT void JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_clearHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
- auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
- hashTableHandler->hashTable()->clear(true);
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
+ std::cout << "VeloxJniWrapper clear hash table id is " << cacheKeyStr << "\n";
+ facebook::velox::exec::HashTableCache::instance()->drop(cacheKeyStr);
Review Comment:
The `hasTable` + `injectTable` sequence is not atomic; concurrent builds for the same key can race and inject twice (depending on Velox cache semantics). Prefer an atomic API (if available) or protect the check/inject with a mutex around the critical section. Also, `std::cout` logging in JNI paths is noisy and hard to control in production—use the project’s logging facility (e.g., `LOG(INFO/WARNING)`) so logs can be filtered and routed consistently.
##########
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:
Using `hashTableId` as the Velox `PlanNode` id risks creating duplicate node IDs when the same build side is reused for multiple join nodes within the same plan. Velox plan node IDs are typically expected to be unique within a plan, and collisions can break plan execution/diagnostics or corrupt any per-node state keyed by id. Prefer keeping `nextPlanNodeId()` for uniqueness and passing the cache key through a dedicated field/config (or embed uniqueness into the id, e.g., prefix/suffix with the generated node id).
##########
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:
This switches the Velox dependency to a personal fork, which is a supply-chain risk and makes builds less reproducible/auditable. Prefer an official upstream/org fork (or a pinned commit SHA in a trusted repo). If you need to keep this temporarily, make the repo/branch configurable via environment variables and default to the canonical upstream so CI/release builds don’t depend on a personal namespace.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -135,8 +135,10 @@ case class BroadcastHashJoinExecTransformer(
override def columnarInputRDDs: Seq[RDD[ColumnarBatch]] = {
val streamedRDD = getColumnarInputRDDs(streamedPlan)
val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ val queryId = GlutenQueryContext.getOrCreateQueryId(sparkContext)
+ val cacheKey = s"$queryId:$buildHashTableId"
if (executionId != null) {
- GlutenDriverEndpoint.collectResources(executionId, buildBroadcastTableId)
+ GlutenDriverEndpoint.collectResources(executionId, cacheKey)
Review Comment:
The cache key is composed ad-hoc with a `:` delimiter. Since this key now crosses multiple layers (driver endpoint, JNI, native cache, and may appear in logs/metrics), it would be safer and easier to evolve if it were built in one place with explicit escaping/sanitization (e.g., a helper that guarantees allowed characters and stable formatting). This prevents future breakage if either component starts emitting or requiring different formats.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1060,6 +1061,13 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
nullptr);
builder->setHashTable(std::move(mainTable));
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(hashTableId, builder->hashTable(), builder->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
Review Comment:
The `hasTable` + `injectTable` sequence is not atomic; concurrent builds for the same key can race and inject twice (depending on Velox cache semantics). Prefer an atomic API (if available) or protect the check/inject with a mutex around the critical section. Also, `std::cout` logging in JNI paths is noisy and hard to control in production—use the project’s logging facility (e.g., `LOG(INFO/WARNING)`) so logs can be filtered and routed consistently.
##########
gluten-arrow/src/main/java/org/apache/gluten/vectorized/NativePlanEvaluator.java:
##########
@@ -113,4 +119,31 @@ public long spill(MemoryTarget self, Spiller.Phase phase, long size) {
private ColumnarBatchOutIterator createOutIterator(Runtime runtime, long itrHandle) {
return new ColumnarBatchOutIterator(runtime, itrHandle);
}
+
+ private static long getExecutionId(TaskContext taskContext) {
+ final String executionId = taskContext.getLocalProperty(SPARK_EXECUTION_ID_KEY);
+ if (executionId == null) {
+ return INVALID_EXECUTION_ID;
+ }
+ try {
+ return Long.parseLong(executionId);
+ } catch (NumberFormatException e) {
+ LOGGER.warn(
+ "Invalid Spark execution id '{}', fallback to {}", executionId, INVALID_EXECUTION_ID);
+ return INVALID_EXECUTION_ID;
+ }
+ }
+
+ private static String getQueryId(TaskContext taskContext) {
+ final String queryId = taskContext.getLocalProperty(GLUTEN_QUERY_ID_KEY);
+ if (queryId != null) {
+ return queryId;
+ }
+
+ final String executionId = taskContext.getLocalProperty(SPARK_EXECUTION_ID_KEY);
+ if (executionId != null) {
+ return "Gluten_Execution_" + executionId;
+ }
+ return "Gluten_Execution_";
+ }
Review Comment:
If both `gluten.query.id` and Spark execution id are missing, the fallback returns the constant `\"Gluten_Execution_\"`, which can cause unrelated tasks/queries to share the same query identifier and collide in any native caching keyed by query id. Use a collision-resistant fallback (e.g., include stageId + taskAttemptId, or generate a UUID) when execution id is unavailable.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3401889370
##########
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:
Defaulting the Velox checkout to a personal fork/branch makes builds non-reproducible and introduces supply-chain risk. The default should point to the project’s canonical Velox repo (or a pinned commit in an approved upstream fork); developers can still override via --velox_repo/--velox_branch for testing.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -105,6 +107,15 @@ trait HashJoinLikeExecTransformer extends BaseJoinExec with TransformSupport {
(right, left)
}
+ protected def canonicalBuildHashTableId(plan: SparkPlan): String = plan match {
+ case b: BroadcastQueryStageExec =>
+ canonicalBuildHashTableId(b.plan)
+ case r: ReusedExchangeExec =>
+ canonicalBuildHashTableId(r.child)
+ case other =>
+ other.id.toString
Review Comment:
canonicalBuildHashTableId currently returns a plain numeric SparkPlan id. This id is later used as the Velox HashJoinNode PlanNodeId (see cpp/velox/substrait/SubstraitToVeloxPlan.cc), which can collide with other PlanNodeIds generated by nextPlanNodeId() (also numeric). Prefixing avoids accidental PlanNodeId collisions while keeping canonicalization for reuse/exchange.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1060,6 +1061,13 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
nullptr);
builder->setHashTable(std::move(mainTable));
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
Review Comment:
Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
}
hashTableBuilders[0]->setHashTable(std::move(mainTable));
+
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(
+ hashTableId,
+ hashTableBuilders[0]->hashTable(),
+ hashTableBuilders[0]->joinHasNullKeys(),
+ defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableBuilders[0]);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_cloneHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(cacheKeyStr)) {
+ cache->injectTable(
+ cacheKeyStr, hashTableHandler->hashTable(), hashTableHandler->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableHandler);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT void JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_clearHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
- auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
- hashTableHandler->hashTable()->clear(true);
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
+ std::cout << "VeloxJniWrapper clear hash table id is " << cacheKeyStr << "\n";
Review Comment:
Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message.
##########
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:
If Spark execution id is unavailable, getVeloxTaskId currently returns the same constant string ("Gluten_Execution_") for all tasks. That can cause unrelated queries/tasks to share the same Velox QueryCtx id and potentially collide in HashTableCache scope. Use a unique fallback derived from stage/task/virtual id instead of an empty suffix.
##########
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:
BroadcastHashJoinContext.buildHashTableId is used as the native hash table/cache identifier (passed into HashJoinBuilder.nativeBuild/clone/clear), but here it’s being rewritten to include the execution id. Meanwhile the Substrait join’s hashtableid is derived from canonicalBuildHashTableId(buildPlan) (without the execution prefix). This key mismatch will prevent cache hits and may also break cleanup because native and JVM caches will be keyed differently.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
}
hashTableBuilders[0]->setHashTable(std::move(mainTable));
+
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
Review Comment:
Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4911630850
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3517867295
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1048,6 +1049,12 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
nullptr);
builder->setHashTable(std::move(mainTable));
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+
+ if (!cache->exist(hashTableId)) {
+ cache->add(hashTableId, builder->hashTable(), builder->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
Review Comment:
`hashTableId` is used for `HashTableCache` lookups, but there is no definition in this function (the `nativeBuild` parameter is currently unnamed as `jstring /*tableId*/`). As-is, this will not compile; it likely needs to derive the cache key from the Java `buildHashTableId` string parameter (e.g., name the parameter and call `jStringToCString`).
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,26 +458,8 @@ 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(),
Review Comment:
`joinNodeId` is declared but never used. With common builds using `-Werror`, this unused variable will break compilation; it also duplicates the `hashtableid()` access unnecessarily.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -394,7 +405,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Spelling: "builded" should be "built" in this comment.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4873119694
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3401313383
##########
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 default Velox source is switched to a personal fork/branch (JkSelf/velox). This makes builds non-reproducible and is risky for CI and downstream users; please keep the default pointing at the project-approved upstream and use the script flags when testing forks.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
}
hashTableBuilders[0]->setHashTable(std::move(mainTable));
+
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(
+ hashTableId,
+ hashTableBuilders[0]->hashTable(),
+ hashTableBuilders[0]->joinHasNullKeys(),
+ defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableBuilders[0]);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_cloneHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(cacheKeyStr)) {
+ cache->injectTable(
+ cacheKeyStr, hashTableHandler->hashTable(), hashTableHandler->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
+
return gluten::getHashTableObjStore()->save(hashTableHandler);
JNI_METHOD_END(kInvalidObjectHandle)
}
JNIEXPORT void JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_clearHashTable( // NOLINT
JNIEnv* env,
jclass,
+ jstring cacheKey,
jlong tableHandler) {
JNI_METHOD_START
- auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler);
- hashTableHandler->hashTable()->clear(true);
+ auto cacheKeyStr = jStringToCString(env, cacheKey);
+ std::cout << "VeloxJniWrapper clear hash table id is " << cacheKeyStr << "\n";
+ facebook::velox::exec::HashTableCache::instance()->drop(cacheKeyStr);
Review Comment:
`std::cout` in `clearHashTable` will emit on every eviction/cleanup and can be very noisy. Use `LOG(INFO)`/`VLOG` instead.
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarBroadcastExchangeExec.scala:
##########
@@ -76,8 +80,14 @@ case class ColumnarBroadcastExchangeExec(mode: BroadcastMode, child: SparkPlan)
child,
longMetric("numOutputRows"),
longMetric("dataSize"),
- metrics.getOrElse("buildThreads", null))
+ metrics.getOrElse("buildThreads", null),
+ queryId)
}
+ logWarning(
Review Comment:
Same issue as above: this is routine tracing (`driver-broadcast-built`) and should not be logged at warning level.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1060,6 +1061,13 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
nullptr);
builder->setHashTable(std::move(mainTable));
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(hashTableId, builder->hashTable(), builder->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
Review Comment:
`std::cout` in JNI code will spam executor logs and bypass the existing logging framework (glog). Prefer `LOG(INFO)` (or `VLOG`) so output can be controlled via log configuration.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -183,6 +190,17 @@ case class BroadcastHashJoinExecTransformer(
// FIXME: Do we have to make build side a RDD?
streamedRDD :+ broadcastRDD
}
+
+ private def extractQueryId(relation: BuildSideRelation): String = relation match {
+ case columnar: ColumnarBuildSideRelation if columnar.queryId.nonEmpty =>
+ columnar.queryId
+ case unsafe: UnsafeColumnarBuildSideRelation if unsafe.queryIdValue.nonEmpty =>
+ unsafe.queryIdValue
+ case _ =>
+ throw new IllegalStateException(
+ s"Missing query id in build-side relation for broadcast hash join " +
+ s"$buildBroadcastTableId, relationType=${relation.getClass.getName}")
+ }
Review Comment:
Throwing here will fail the query if a build-side relation is created without a non-empty `queryId` (e.g., default argument path or unexpected BuildSideRelation implementation). Consider falling back to the driver-scoped `GlutenQueryContext` query id (and warn) rather than hard-failing.
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarBroadcastExchangeExec.scala:
##########
@@ -57,11 +57,15 @@ case class ColumnarBroadcastExchangeExec(mode: BroadcastMode, child: SparkPlan)
@transient
override lazy val relationFuture: java.util.concurrent.Future[broadcast.Broadcast[Any]] = {
+ val queryId = org.apache.gluten.execution.GlutenQueryContext.getOrCreateQueryId(sparkContext)
SQLExecution.withThreadLocalCaptured[broadcast.Broadcast[Any]](
session,
BroadcastExchangeExec.executionContext) {
try {
SparkShimLoader.getSparkShims.setJobDescriptionOrTagForBroadcastExchange(sparkContext, this)
+ logWarning(
Review Comment:
`logWarning` is used for normal broadcast lifecycle tracing. This is expected to run frequently and will pollute warning logs; please switch these trace logs to `logInfo`/`logDebug`.
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarBroadcastExchangeExec.scala:
##########
@@ -86,6 +96,9 @@ case class ColumnarBroadcastExchangeExec(mode: BroadcastMode, child: SparkPlan)
sparkContext,
relation.asInstanceOf[Any])
}
+ logWarning(
Review Comment:
Same issue as above: `driver-broadcast-published` is normal operation and should not be warning-level.
##########
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:
Potential key mismatch for BHJ hash table caching: the build side injects into Velox `HashTableCache` using `BroadcastHashJoinContext.buildHashTableId` (now `cacheKey = s"$queryId:$buildHashTableId"`), but the probe side uses `JoinRel.hashtableid` (set via the `hashTableId` argument passed to `JoinUtils.createJoinRel`). With the current code, `hashTableId` is only `canonicalBuildHashTableId(buildPlan)` (no queryId prefix), so the HashJoinNode key will not match the cache key used during build, and cache lookup will fail.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -135,16 +137,21 @@ case class BroadcastHashJoinExecTransformer(
override def columnarInputRDDs: Seq[RDD[ColumnarBatch]] = {
val streamedRDD = getColumnarInputRDDs(streamedPlan)
val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+ val broadcast = buildPlan.executeBroadcast[BuildSideRelation]()
+ val queryId = extractQueryId(broadcast.value)
+ val cacheKey = s"$queryId:$buildHashTableId"
+ logWarning(
Review Comment:
`logWarning` is used for routine tracing (cache key creation). This will generate warning-level noise for every broadcast hash join even when nothing is wrong; please use `logInfo`/`logDebug` instead.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
}
hashTableBuilders[0]->setHashTable(std::move(mainTable));
+
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+ if (!cache->hasTable(hashTableId)) {
+ std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n";
+ cache->injectTable(
+ hashTableId,
+ hashTableBuilders[0]->hashTable(),
+ hashTableBuilders[0]->joinHasNullKeys(),
+ defaultLeafVeloxMemoryPool());
+ }
Review Comment:
Same as above: please avoid `std::cout` for routine tracing in JNI and use the repo's logger so it can be filtered.
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -72,6 +72,18 @@ const std::string kHiveDefaultPartition = "__HIVE_DEFAULT_PARTITION__";
} // namespace
+namespace {
+std::string getVeloxTaskId(const SparkTaskInfo& taskInfo) {
+ if (!taskInfo.queryId.empty()) {
+ return taskInfo.queryId;
+ }
+ if (taskInfo.executionId != -1) {
+ return fmt::format("Gluten_Execution_{}", std::to_string(taskInfo.executionId));
+ }
+ return fmt::format("Gluten_Execution_{}", "");
+}
Review Comment:
When neither `queryId` nor `executionId` is available, `getVeloxTaskId` currently returns `Gluten_Execution_` with an empty suffix, which is not unique and not very helpful for debugging/Velox context naming. Use the existing stage/task/vId fields as a deterministic fallback.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -867,25 +868,36 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi with Logging {
val buildThreadsValue = if (rawThreads < 1) 1 else rawThreads
buildThreads += buildThreadsValue
- if (useOffheapBroadcastBuildRelation) {
- TaskResources.runUnsafe {
- UnsafeColumnarBuildSideRelation(
+ val relation =
+ if (useOffheapBroadcastBuildRelation) {
+ TaskResources.runUnsafe {
+ UnsafeColumnarBuildSideRelation(
+ newOutput,
+ serialized.flatMap(_.offHeapData().asScala),
+ mode,
+ newBuildKeys,
+ offload,
+ buildThreadsValue,
+ queryId)
+ }
+ } else {
+ ColumnarBuildSideRelation(
newOutput,
- serialized.flatMap(_.offHeapData().asScala),
+ serialized.flatMap(_.onHeapData().asScala).toArray,
mode,
newBuildKeys,
offload,
- buildThreadsValue)
+ buildThreadsValue,
+ queryId)
}
- } else {
- ColumnarBuildSideRelation(
- newOutput,
- serialized.flatMap(_.onHeapData().asScala).toArray,
- mode,
- newBuildKeys,
- offload,
- buildThreadsValue)
- }
+ logWarning(
Review Comment:
`logWarning` is used for normal relation creation tracing. This is not an exceptional condition and may be extremely noisy in production; prefer `logInfo`/`logDebug`.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4629800684
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4655658353
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3526284460
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -489,10 +471,11 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
leftNode,
rightNode,
getJoinOutputType(leftNode, rightNode, joinType),
+ true,
false,
false,
- joinHasNullKeys,
- opaqueSharedHashTable);
+ nullptr,
+ sJoin.hashtableid());
Review Comment:
This BHJ branch no longer retrieves the prebuilt hash table from the ObjectStore (old `getJoin(hashTableId)` path) and instead passes `nullptr` plus `sJoin.hashtableid()` into `HashJoinNode`, which makes correctness depend on the Velox `HashTableCache` being populated for that key. However, the executor deserialization path (`SerializedBroadcastHashTable.deserialize()` -> `HashJoinBuilder.deserializeHashTableDirect`) currently just calls `gluten::deserializeHashTable(...)` and returns a builder handle without inserting into `HashTableCache` (see `cpp/velox/jni/VeloxJniWrapper.cc:1198-1203`, `cpp/velox/jni/JniHashTable.cc:201-246`). That means Spark-native broadcast (driver builds/serializes, executor deserializes) can end up with a missing cache entry for `hashtableid`.
Consider either (a) extending the deserialize JNI path to add the resulting hash table into `HashTableCache` using the broadcast/build hash table id, or (b) adding a fallback here to populate the cache from the existing `getJoin(hashTableId)` handle when the cache is empty.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -397,7 +408,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Comment typo: "builded" should be "built".
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3541440483
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -489,10 +471,11 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
leftNode,
rightNode,
getJoinOutputType(leftNode, rightNode, joinType),
+ true,
false,
false,
- joinHasNullKeys,
- opaqueSharedHashTable);
+ nullptr,
+ sJoin.hashtableid());
Review Comment:
`joinNodeId` is computed but never used, and the current code calls `nextPlanNodeId()` twice when `hashtableid` is empty (once for `joinNodeId`, once for the actual HashJoinNode). This can (a) fail compilation under `-Werror=unused-variable`, (b) skip/consume a plan node id unexpectedly, and (c) still pass an empty cache key (`sJoin.hashtableid()`) into the HashJoinNode, which risks cache-key collisions if HashTableCache is used.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -397,7 +408,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Spelling in comment: "builded" should be "built".
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4687609316
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4747939866
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4873475239
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4911098303
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4656834929
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4921177325
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3386149611
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -448,29 +448,22 @@ 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;
-
+ const auto& hashTableId = sJoin.hashtableid();
+ auto useHashTableCache = true;
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;
+ useHashTableCache = false;
}
+ size_t pos = hashTableId.find(':');
+ auto planNodeId = hashTableId.substr(pos + 1);
+ std::cout << "the cacheKey is " << hashTableId << " the planNode id is " << planNodeId << std::endl;
Review Comment:
Using `std::cout` for debug logging will spam executor logs and bypasses the project's logging configuration/levels. Please remove this line (or switch to an existing logging macro that can be gated by verbosity).
##########
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` now returns the same ID for all Spark tasks that share an SQL executionId. Velox `Task::create` expects task IDs to be unique within a process; reusing the same ID across concurrent Spark tasks can lead to task registry collisions, incorrect stats attribution, or runtime failures. Keep the executionId in the string if needed for cache scoping, but also include stageId/taskId/vId (or otherwise guarantee uniqueness).
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -448,29 +448,22 @@ 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;
-
+ const auto& hashTableId = sJoin.hashtableid();
+ auto useHashTableCache = true;
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;
+ useHashTableCache = false;
}
+ size_t pos = hashTableId.find(':');
+ auto planNodeId = hashTableId.substr(pos + 1);
+ std::cout << "the cacheKey is " << hashTableId << " the planNode id is " << planNodeId << std::endl;
+ const auto joinNodeId = hashTableId.empty() ? nextPlanNodeId() : planNodeId;
+
Review Comment:
`joinNodeId` is being set to `planNodeId` parsed from the Spark-side hashTableId (e.g. a numeric Spark plan id). `SubstraitToVeloxPlanConverter::nextPlanNodeId()` also generates sequential numeric IDs starting from 0, so this can easily collide with already-assigned Velox plan node IDs and produce an invalid plan (duplicate PlanNodeId). The join node ID must be guaranteed unique within the converted Velox plan while still allowing HashTableCache lookup.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -986,6 +987,7 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native
const auto abandonHashBuildDedupMinPct =
queryConf.get<uint32_t>(kAbandonDedupHashMapMinPct, kAbandonDedupHashMapMinPctDefault);
const auto hashTableId = jStringToCString(env, tableId);
+ std::cout << "the cacheKey in nativeBuild is " << hashTableId << std::endl;
Review Comment:
`std::cout` debug output in JNI code will produce noisy executor logs and is not controllable via log levels. Please remove this line (or replace with an existing logging facility that can be configured/disabled).
##########
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 build to a personal Velox fork/branch (`github.com/JkSelf/velox.git`) is a supply-chain risk and makes builds non-reproducible for CI and other contributors. The default repo/branch should remain an official upstream (or the existing IBM fork used by this project), with developer overrides handled via `--velox_repo/--velox_branch` or `UPSTREAM_VELOX_PR_ID`.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -602,6 +602,7 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenResolvedDataSourceSuite]
enableSuite[GlutenSaveLoadSuite]
enableSuite[GlutenTableScanSuite]
+ .includeByPrefix("Caching")
Review Comment:
`.includeByPrefix("Caching")` limits `GlutenTableScanSuite` to only tests whose names start with "Caching", which substantially reduces coverage for table-scan behavior in the Velox CI test matrix. If the goal is to add/enable specific caching-related cases, consider including only the new tests (without filtering out the rest of the suite) or moving the filtering behind a dedicated CI profile.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4678600196
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4677215154
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4748036859
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4749527867
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3541029854
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,26 +458,8 @@ 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
Review Comment:
`joinNodeId` is computed but never used. When `hashTableId` is empty this still calls `nextPlanNodeId()` and increments `planNodeId_`, so the subsequent `HashJoinNode` ends up skipping an ID (and potentially diverging from other plan-node numbering assumptions). Remove the unused variable (or use it) to avoid the unintended side effect.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -397,7 +408,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Typo in comment: "builded" should be "built".
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4646122159
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4561284995
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4656134900
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3371307220
##########
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:
The default Velox source is switched to a personal fork/branch. For reproducible builds and supply-chain safety, the default should remain an official upstream (or org-controlled) repository; developers can still override via --velox_repo/--velox_branch when testing.
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -72,6 +72,19 @@ 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_Stage_{}_TID_{}_VTID_{}",
+ std::to_string(taskInfo.stageId),
+ std::to_string(taskInfo.taskId),
+ std::to_string(taskInfo.vId));
+}
Review Comment:
Using only Spark SQL executionId as the Velox Task id will be identical across all Spark tasks belonging to the same SQL execution. Velox Task ids are typically required to be unique per running task; collisions can lead to task registry conflicts or cross-task state bleed. Consider keeping per-task uniqueness and (optionally) prefixing with executionId for grouping.
##########
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:
`joinNodeId` is set to `hashtableid` when provided. If `hashtableid` is a numeric SparkPlan id (e.g., "5"), it can collide with ids produced by `nextPlanNodeId()` (which are sequential numeric strings starting from 0), violating Velox's expectation that PlanNodeIds are unique within a plan. This needs a namespacing strategy (e.g., keep a non-numeric prefix like the previous "BuiltHashTable-", or prefix all `nextPlanNodeId()` ids) that is consistent with the HashTableCache key used at build time.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -392,7 +403,7 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def hashJoinType: JoinType = joinType
// Unique ID for builded hash table
- lazy val buildHashTableId: String = "BuiltHashTable-" + buildPlan.id
+ lazy val buildHashTableId: String = canonicalBuildHashTableId(buildPlan)
Review Comment:
`buildHashTableId` now resolves to the underlying SparkPlan numeric id. This id is propagated into Substrait and then reused as a Velox PlanNodeId / cache key, where purely-numeric ids can collide with other generated plan node ids (see SubstraitToVeloxPlanConverter::nextPlanNodeId()) and are also not obviously namespaced. Consider restoring a stable, non-numeric prefix (e.g., "BuiltHashTable-") while still canonicalizing through BroadcastQueryStageExec/ReusedExchangeExec.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -106,7 +106,7 @@ case class BroadcastHashJoinExecTransformer(
isNullAwareAntiJoin) {
// Unique ID for built table
- lazy val buildBroadcastTableId: String = buildPlan.id.toString
+ lazy val buildBroadcastTableId: String = canonicalBuildHashTableId(buildPlan)
Review Comment:
`buildBroadcastTableId` is now the canonical SparkPlan numeric id, which is later used as the broadcast hash table id/cache key. As with the Substrait-side usage, purely-numeric ids can collide with other Velox plan node ids and are not namespaced. Consider using a stable prefixed form (e.g., "BuiltHashTable-<canonicalId>") while still canonicalizing through BroadcastQueryStageExec/ReusedExchangeExec.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4748135511
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4688188773
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4689165095
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4687266067
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12163:
URL: https://github.com/apache/gluten/pull/12163#issuecomment-4688816737
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][TEST]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3541424344
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,26 +458,8 @@ 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(),
Review Comment:
`joinNodeId` is computed but never used, and `HashJoinNode` still gets a fresh `nextPlanNodeId()` as its plan node id. This leaves an unused variable (can fail builds with -Werror) and also defeats the intent of using a stable id for BHJ hash table caching.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -397,7 +408,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Typo in comment: "builded" should be "built".
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL]Use Velox's HashTableCache to cache the BHJ's HashTable [gluten]
Posted by "JkSelf (via GitHub)" <gi...@apache.org>.
JkSelf merged PR #12163:
URL: https://github.com/apache/gluten/pull/12163
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org