[GLUTEN-10660][VL] Fix passing Velox session configurations on hash build optimizations#11134
Conversation
|
Run Gluten Clickhouse CI on x86 |
zhouyuan
changed the title
| .foreach(entry => nativeConfMap.put(entry._1, entry._2)) | ||
|
|
||
| // Backend's dynamic session conf only. | ||
| val confPrefix = prefixOf(backendName) |
There was a problem hiding this comment.
Could you change this method name from prefixOf to sessionPrefixOf?
There was a problem hiding this comment.
The code should be work for the hash build dedup feature. However for now Gluten actually does not have a clear boundary on session level vs. static config, I guess we may need some refactor on this part in future work.
| std::to_string(veloxCfg_->get<int32_t>(kAbandonBuildNoDupHashMinRows, 100000)); | ||
| configs[velox::core::QueryConfig::kAbandonBuildNoDupHashMinPct] = | ||
| std::to_string(veloxCfg_->get<int32_t>(kAbandonBuildNoDupHashMinPct, 0)); | ||
| std::to_string(static_cast<int32_t>(veloxCfg_->get<double>(kAbandonBuildNoDupHashMinPct, 0))); |
There was a problem hiding this comment.
Why not just change the type of original configuration?
There was a problem hiding this comment.
it's mostly due to the config name has a percentile so we change that to double based config when enable this feature. I have modified to use int based and update the comments
Signed-off-by: Yuan <yuanzhou@apache.org>
zhouyuan
force-pushed
the
wip_fix_velox_config
branch
from
4469d8b to
765c5c4
Compare
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
Signed-off-by: Yuan <yuanzhou@apache.org>
zhouyuan
force-pushed
the
wip_fix_velox_config
branch
from
97a0328 to
51bed04
Compare
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
beliefer
left a comment
beliefer
left a comment
There was a problem hiding this comment.
It looks good to me if the session configurations used by Velox.
|
The fail test seems not related to this PR. It is failed in other PR too. |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
Thanks. Assume the CI failure is not related.
4 participants
What changes are proposed in this pull request?
some important configurations on Velox hashmap dedup are not passed to Velox backend
How was this patch tested?
locally verified
Related issue: #10660