[GLUTEN-11749][VL] Throw exception if cannot get the pre-built hash table from ObjectStore correctly#11898
[GLUTEN-11749][VL] Throw exception if cannot get the pre-built hash table from ObjectStore correctly#11898JkSelf wants to merge 3 commits intoapache:mainfrom
Conversation
JkSelf
changed the title
| << "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; | ||
| if (isHashTableBuildOncePerExecutor) { |
There was a problem hiding this comment.
Could you read the config from the member veloxCfg_ of SubstraitToVeloxPlan?
|
|
||
| LOG(INFO) << "Successfully retrieved and aliased HashTable for reuse. ID: " << hashTableId; | ||
| } catch (const std::exception& e) { | ||
| LOG(ERROR) |
There was a problem hiding this comment.
LOG(ERROR) does not throw exception, if the config is false, please interrupt here. throw e. Could you not use try catch for normal execution path?
There was a problem hiding this comment.
When this configuration is enabled, the hash table must be retrieved from the ObjectStore. If retrieval fails, an exception should be thrown on the Gluten side rather than letting it propagate to the Velox side, consistent with the approach in PR #11749. If configuration is disabled, we already set opaqueSharedHashTable to null here and no need further process.
|
Run Gluten Clickhouse CI on x86 |
| << ". Falling back to building new table. To ensure correct results, please verify that spark.gluten.velox.buildHashTableOncePerExecutor.enabled is set to false."; | ||
| opaqueSharedHashTable = nullptr; | ||
| if (hashTableBuildOncePerExecutorEnabled) { | ||
| std::cout << "the hashTableBuildOncePerExecutorEnabled is set" << "\n"; |
There was a problem hiding this comment.
Do you need the log?
| " to false as workaround."); | ||
| } | ||
| } else { | ||
| std::cout << "the hashTableBuildOncePerExecutorEnabled is false" << "\n"; |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
2 participants
What changes are proposed in this pull request?
When enabling
HashTableBuildOncePerExecutor, we need use the pre-built hash table. If not, it should throw exception directly.How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Related issue: #11749