Copilot commented on code in PR #12487:
URL: https://github.com/apache/gluten/pull/12487#discussion_r3557246020
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,6 +458,21 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
} else if (
sJoin.has_advanced_extension() &&
SubstraitParser::configSetInOptimization(sJoin.advanced_extension(),
"isBHJ=")) {
+ const std::string hashTableId = sJoin.hashtableid();
+ bool useHashTableCache = false;
+ if (!hashTableId.empty()) {
+ try {
+ const auto handle = getJoin(hashTableId);
+ if (handle != 0) {
+ auto hashTableBuilder =
ObjectStore::retrieve<gluten::HashTableBuilder>(handle);
+ useHashTableCache = (hashTableBuilder != nullptr);
Review Comment:
This new logic introduces a JNI call via getJoin(hashTableId).
JniHashTableContext::callJavaGet currently creates a jstring with
env->NewStringUTF but never deletes the local reference, and it also doesn’t
check/clear pending Java exceptions after CallStaticLongMethod. With this new
call site, repeated plan conversions in a single JNI frame can accumulate local
refs or silently proceed with a pending exception; consider fixing callJavaGet
to DeleteLocalRef(s) (or use a scoped local ref helper) and handle JNI
exceptions explicitly.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,6 +458,21 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
} else if (
sJoin.has_advanced_extension() &&
SubstraitParser::configSetInOptimization(sJoin.advanced_extension(),
"isBHJ=")) {
+ const std::string hashTableId = sJoin.hashtableid();
+ bool useHashTableCache = false;
+ if (!hashTableId.empty()) {
+ try {
+ const auto handle = getJoin(hashTableId);
Review Comment:
The new cache-selection behavior (useHashTableCache depends on looking up
the hash table in VeloxBroadcastBuildSideCache/ObjectStore) appears to be the
core of the leak fix, but there’s no regression test coverage ensuring the plan
conversion correctly toggles hash-table caching when the cache key is
missing/evicted or when the handle is stale. Adding a targeted test (e.g., in
existing Velox hash join suites) for the cache-miss path would help prevent
future regressions of the SQLExecutionEnd memory-leak scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
