Skip to content

[VL] Add hash table memory usage metric when enabling spark.gluten.velox.buildHashTableOncePerExecutor.enabled#12528

Merged
JkSelf merged 1 commit into
apache:mainfrom
JkSelf:bhj-hashtable-usage
Jul 21, 2026
Merged

[VL] Add hash table memory usage metric when enabling spark.gluten.velox.buildHashTableOncePerExecutor.enabled#12528
JkSelf merged 1 commit into
apache:mainfrom
JkSelf:bhj-hashtable-usage

Conversation

@JkSelf

@JkSelf JkSelf commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Add hash table memory usage metric when enabling spark.gluten.velox.buildHashTableOncePerExecutor.enabled.

image

How was this patch tested?

local verified

Was this patch authored or co-authored using generative AI tooling?

Copilot AI review requested due to automatic review settings July 16, 2026 03:34
@github-actions github-actions Bot added the VELOX label Jul 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Spark SQL metric to report retained hash table memory usage for Velox broadcast hash joins when the “build hash table once per executor” path is used, wiring the value from native Velox hash table allocation tracking through JNI into Spark metrics.

Changes:

  • Track hash table retained bytes in native (HashTableBuilder) and expose it via a new JNI/Java API (HashJoinBuilder.getHashTableMemoryUsage).
  • Populate the memory usage value during hash table build (including parallel-build subtables) and report it into a new Spark SizeMetric (hashTableMemorySize).
  • Thread the new metric through broadcast hash join context/plumbing so it’s recorded when building the executor-side cached hash table.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cpp/velox/operators/hashjoin/HashTableBuilder.h Adds storage/accessors for a cached hash table memory usage value.
cpp/velox/jni/VeloxJniWrapper.cc Captures allocated bytes during native build and adds a JNI entrypoint to query the metric.
cpp/velox/jni/JniHashTable.h Declares hashTableMemoryUsage(...) native helper.
cpp/velox/jni/JniHashTable.cc Implements hashTableMemoryUsage(...) accessor (note: needs fallback for deserialized handles).
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala Updates the new SQL metric when building a cached broadcast hash table.
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala Threads the new metric through BroadcastHashJoinContext.
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxMetricsApi.scala Registers hashTableMemorySize as a new Spark size metric.
backends-velox/src/main/java/org/apache/gluten/vectorized/HashJoinBuilder.java Adds the new native method declaration for querying hash table memory usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +183 to +186
int64_t hashTableMemoryUsage(std::shared_ptr<HashTableBuilder> builder) {
VELOX_CHECK_NOT_NULL(builder, "Hash table builder cannot be null");
return builder->hashTableMemoryUsage();
}
Copilot AI review requested due to automatic review settings July 20, 2026 06:00
@JkSelf
JkSelf force-pushed the bhj-hashtable-usage branch from 38855b9 to 6f63592 Compare July 20, 2026 06:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Comment on lines +89 to +96
// #region debug-point bhj-hash-memory-size-cache-check
val cached = buildSideRelationCache.getIfPresent(broadcastContext.buildHashTableId)
logWarning(
s"[debug][bhj-hash-memory-size] getOrBuildBroadcastHashTable " +
s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
s"cacheHit=${cached != null}, " +
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
// #endregion debug-point bhj-hash-memory-size-cache-check
Comment on lines +102 to +106
// #region debug-point bhj-hash-memory-size-loader-enter
logWarning(
s"[debug][bhj-hash-memory-size] cache loader entered " +
s"for buildHashTableId=${broadcastContext.buildHashTableId}")
// #endregion debug-point bhj-hash-memory-size-loader-enter
Comment on lines +114 to +123
// #region debug-point bhj-hash-memory-size-value
val hashTableMemoryUsage = HashJoinBuilder.getHashTableMemoryUsage(pointer)
logWarning(
s"[debug][bhj-hash-memory-size] built hash table " +
s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
s"pointer=$pointer, " +
s"hashTableMemoryUsage=$hashTableMemoryUsage, " +
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
broadcastContext.hashTableMemorySizeMetric.foreach(_ += hashTableMemoryUsage)
// #endregion debug-point bhj-hash-memory-size-value
Comment on lines +44 to +51
// #region debug-point bhj-hash-memory-size-branch
logWarning(
s"[debug][bhj-hash-memory-size] VeloxBroadcastBuildSideRDD " +
s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
s"relationClass=${relationCopy.getClass.getName}, " +
s"isBNL=$isBNL, " +
s"offload=$offload")
// #endregion debug-point bhj-hash-memory-size-branch
Comment on lines +44 to +49
// #region debug-point bhj-hash-memory-size-serialized-deserialize
logWarning(
s"[debug][bhj-hash-memory-size] VeloxSerializedBroadcastRDD " +
s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
s"deserializeMetricDefined=${broadcastContext.deserializeHashTableTimeMetric.isDefined}")
// #endregion debug-point bhj-hash-memory-size-serialized-deserialize
Comment on lines +187 to +200
// #region debug-point bhj-hash-memory-size-driver-attach
val cacheCodeSource = Option(
org.apache.gluten.execution.VeloxBroadcastBuildSideCache
.getClass
.getProtectionDomain
.getCodeSource
).map(_.getLocation).map(_.toString).getOrElse("unknown")
logWarning(
s"[debug][bhj-hash-memory-size] BHJ driver context " +
s"buildHashTableId=$buildBroadcastTableId, " +
s"hashTableMemorySizeMetricDefined=${context.hashTableMemorySizeMetric.isDefined}, " +
s"buildHashTableTimeMetricDefined=${context.buildHashTableTimeMetric.isDefined}, " +
s"cacheCodeSource=$cacheCodeSource")
// #endregion debug-point bhj-hash-memory-size-driver-attach
Copilot AI review requested due to automatic review settings July 20, 2026 06:47
@JkSelf
JkSelf force-pushed the bhj-hashtable-usage branch from 6f63592 to 1be6538 Compare July 20, 2026 06:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

cpp/velox/jni/JniHashTable.cc:186

  • hashTableMemoryUsage(builder) currently just returns HashTableBuilder::hashTableMemoryUsage_, but that field is only set by nativeBuild. Hash tables created via deserializeHashTableDirect (and any other path that doesn't call setHashTableMemoryUsage) will therefore report 0, which contradicts the API comment (“retained bytes ...”). Consider deriving the value from builder->hashTable()->allocatedBytes() (or using it as a fallback) so the JNI/Java API works for all hash table handles.
int64_t hashTableMemoryUsage(std::shared_ptr<HashTableBuilder> builder) {
  VELOX_CHECK_NOT_NULL(builder, "Hash table builder cannot be null");
  return builder->hashTableMemoryUsage();
}

@JkSelf
JkSelf force-pushed the bhj-hashtable-usage branch from 1be6538 to e2ac62f Compare July 21, 2026 02:55
@JkSelf
JkSelf merged commit ce7aa63 into apache:main Jul 21, 2026
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants