Copilot commented on code in PR #12926: URL: https://github.com/apache/gluten/pull/12926#discussion_r3881938101
########## ep/build-velox/src/get-velox.sh: ########## @@ -18,8 +18,8 @@ set -exu CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd) VELOX_REPO=https://github.com/IBM/velox.git -VELOX_BRANCH=dft-2026_08_26 -VELOX_ENHANCED_BRANCH=ibm-2026_08_26 +VELOX_BRANCH=dft-2026_08_26-refactor-hashbuilder +VELOX_ENHANCED_BRANCH=ibm-2026_08_26-refactor-hashbuilder Review Comment: This pins the build to feature branches, which can be force-pushed or deleted and may break reproducibility/CI over time. Prefer pinning to an immutable commit SHA (or a release tag) and, if needed, keep the branch name only as a developer override. ########## cpp/velox/operators/hashjoin/HashTableBuilder.cc: ########## @@ -17,361 +17,51 @@ #include "operators/hashjoin/HashTableBuilder.h" -#include <algorithm> - -#include <iostream> - -#include "velox/exec/OperatorUtils.h" - namespace gluten { -namespace { -facebook::velox::RowTypePtr hashJoinTableType( - const std::vector<facebook::velox::core::FieldAccessTypedExprPtr>& joinKeys, - const facebook::velox::RowTypePtr& inputType, - bool includeDependents) { - const auto numKeys = joinKeys.size(); - - std::vector<std::string> names; - names.reserve(includeDependents ? inputType->size() : numKeys); - std::vector<facebook::velox::TypePtr> types; - types.reserve(includeDependents ? inputType->size() : numKeys); - std::unordered_set<uint32_t> keyChannelSet; - keyChannelSet.reserve(inputType->size()); - - for (int i = 0; i < numKeys; ++i) { - auto& key = joinKeys[i]; - auto channel = facebook::velox::exec::exprToChannel(key.get(), inputType); - keyChannelSet.insert(channel); - names.emplace_back(inputType->nameOf(channel)); - types.emplace_back(inputType->childAt(channel)); - } - - if (!includeDependents) { - return ROW(std::move(names), std::move(types)); - } - - for (auto i = 0; i < inputType->size(); ++i) { - if (keyChannelSet.find(i) == keyChannelSet.end()) { - names.emplace_back(inputType->nameOf(i)); - types.emplace_back(inputType->childAt(i)); - } - } - return ROW(std::move(names), std::move(types)); -} - -bool isLeftNullAwareJoinWithFilter(facebook::velox::core::JoinType joinType, bool nullAware, bool withFilter) { - return (isAntiJoin(joinType) || isLeftSemiProjectJoin(joinType) || isLeftSemiFilterJoin(joinType)) && nullAware && - withFilter; -} -} // namespace +using namespace facebook::velox; Review Comment: A `using namespace` at namespace scope (even in a .cc) increases the risk of symbol collisions and makes call sites less explicit during future refactors. Consider replacing it with targeted aliases (e.g., `namespace exec = facebook::velox::exec;`) or qualifying the few referenced types (`core::JoinType`, `RowTypePtr`, etc.). ########## cpp/velox/operators/hashjoin/HashTableBuilder.cc: ########## @@ -17,361 +17,51 @@ #include "operators/hashjoin/HashTableBuilder.h" -#include <algorithm> - -#include <iostream> - -#include "velox/exec/OperatorUtils.h" - namespace gluten { -namespace { -facebook::velox::RowTypePtr hashJoinTableType( - const std::vector<facebook::velox::core::FieldAccessTypedExprPtr>& joinKeys, - const facebook::velox::RowTypePtr& inputType, - bool includeDependents) { - const auto numKeys = joinKeys.size(); - - std::vector<std::string> names; - names.reserve(includeDependents ? inputType->size() : numKeys); - std::vector<facebook::velox::TypePtr> types; - types.reserve(includeDependents ? inputType->size() : numKeys); - std::unordered_set<uint32_t> keyChannelSet; - keyChannelSet.reserve(inputType->size()); - - for (int i = 0; i < numKeys; ++i) { - auto& key = joinKeys[i]; - auto channel = facebook::velox::exec::exprToChannel(key.get(), inputType); - keyChannelSet.insert(channel); - names.emplace_back(inputType->nameOf(channel)); - types.emplace_back(inputType->childAt(channel)); - } - - if (!includeDependents) { - return ROW(std::move(names), std::move(types)); - } - - for (auto i = 0; i < inputType->size(); ++i) { - if (keyChannelSet.find(i) == keyChannelSet.end()) { - names.emplace_back(inputType->nameOf(i)); - types.emplace_back(inputType->childAt(i)); - } - } - return ROW(std::move(names), std::move(types)); -} - -bool isLeftNullAwareJoinWithFilter(facebook::velox::core::JoinType joinType, bool nullAware, bool withFilter) { - return (isAntiJoin(joinType) || isLeftSemiProjectJoin(joinType) || isLeftSemiFilterJoin(joinType)) && nullAware && - withFilter; -} -} // namespace +using namespace facebook::velox; HashTableBuilder::HashTableBuilder( - facebook::velox::core::JoinType joinType, + core::JoinType joinType, bool nullAware, bool withFilter, int64_t bloomFilterPushdownSize, - const std::vector<facebook::velox::core::FieldAccessTypedExprPtr>& joinKeys, + const std::vector<core::FieldAccessTypedExprPtr>& joinKeys, const std::vector<column_index_t>& filterInputChannels, bool filterPropagatesNulls, - const facebook::velox::RowTypePtr& inputType, - facebook::velox::memory::MemoryPool* pool, + const RowTypePtr& inputType, + memory::MemoryPool* pool, uint32_t minTableRowsForParallelJoinBuild, uint32_t joinBuildVectorHasherMaxNumDistinct, uint32_t abandonHashBuildDedupMinRows, - uint32_t abandonHashBuildDedupMinPct) - : joinType_{joinType}, - nullAware_{nullAware}, - withFilter_(withFilter), - keyChannelMap_(joinKeys.size()), - inputType_(inputType), - bloomFilterPushdownSize_(bloomFilterPushdownSize), - pool_(pool), - minTableRowsForParallelJoinBuild_(minTableRowsForParallelJoinBuild), - joinBuildVectorHasherMaxNumDistinct_(joinBuildVectorHasherMaxNumDistinct), - abandonHashBuildDedupMinRows_(abandonHashBuildDedupMinRows), - abandonHashBuildDedupMinPct_(abandonHashBuildDedupMinPct), - filterPropagatesNulls_(filterPropagatesNulls) { - dropDuplicates_ = - !withFilter_ && (isLeftSemiFilterJoin(joinType_) || isLeftSemiProjectJoin(joinType_) || isAntiJoin(joinType_)); - const auto numKeys = joinKeys.size(); - keyChannels_.reserve(numKeys); - - for (int i = 0; i < numKeys; ++i) { - auto& key = joinKeys[i]; - auto channel = facebook::velox::exec::exprToChannel(key.get(), inputType_); - keyChannelMap_[channel] = i; - keyChannels_.emplace_back(channel); - } - - // Identify the non-key build side columns and make a decoder for each. - if (!dropDuplicates_) { - const int32_t numDependents = inputType_->size() - numKeys; - if (numDependents > 0) { - // Number of join keys (numKeys) may be less then number of input columns - // (inputType->size()). In this case numDependents is negative and cannot - // be used to call 'reserve'. This happens when we join different probe - // side keys with the same build side key: SELECT * FROM t LEFT JOIN u ON - // t.k1 = u.k AND t.k2 = u.k. - dependentChannels_.reserve(numDependents); - decoders_.reserve(numDependents); - } - for (auto i = 0; i < inputType->size(); ++i) { - if (keyChannelMap_.find(i) == keyChannelMap_.end()) { - dependentChannels_.emplace_back(i); - decoders_.emplace_back(std::make_unique<facebook::velox::DecodedVector>()); - } - } - } - - tableType_ = hashJoinTableType(joinKeys, inputType, !dropDuplicates_); - setupTable(); - - if (isAntiJoin(joinType_) && withFilter_ && filterPropagatesNulls_) { - setupFilterForAntiJoins(filterInputChannels); - } -} - -void HashTableBuilder::setupFilterForAntiJoins(const std::vector<column_index_t>& filterInputChannels) { - VELOX_DCHECK(std::is_sorted(dependentChannels_.begin(), dependentChannels_.end())); - - for (auto channel : filterInputChannels) { - auto keyIter = keyChannelMap_.find(channel); - if (keyIter != keyChannelMap_.end()) { - keyFilterChannels_.push_back(keyIter->second); - continue; - } - - auto dependentIter = std::lower_bound(dependentChannels_.begin(), dependentChannels_.end(), channel); - if (dependentIter == dependentChannels_.end() || *dependentIter != channel) { - continue; - } - dependentFilterChannels_.push_back(dependentIter - dependentChannels_.begin()); - } -} - -void HashTableBuilder::removeInputRowsForAntiJoinFilter() { - bool changed = false; - auto* rawActiveRows = activeRows_.asMutableRange().bits(); - - auto removeNulls = [&](facebook::velox::DecodedVector& decoded) { - if (decoded.mayHaveNulls()) { - changed = true; - facebook::velox::bits::andBits(rawActiveRows, decoded.nulls(&activeRows_), 0, activeRows_.end()); - } - }; - - for (auto channel : keyFilterChannels_) { - removeNulls(uniqueTable_->hashers()[channel]->decodedVector()); - } - for (auto channel : dependentFilterChannels_) { - removeNulls(*decoders_[channel]); - } - - if (changed) { - activeRows_.updateBounds(); - } -} - -bool HashTableBuilder::abandonHashBuildDedupEarly(int64_t numDistinct) const { - VELOX_CHECK(dropDuplicates_); - return numHashInputRows_ > abandonHashBuildDedupMinRows_ && - (100 * numDistinct / numHashInputRows_) >= abandonHashBuildDedupMinPct_; + uint32_t abandonHashBuildDedupMinPct) { + exec::JoinTableBuilder::Options options; + options.joinType = joinType; + options.nullAware = nullAware; + options.withFilter = withFilter; + options.inputType = inputType; + options.joinKeys = joinKeys; + options.minTableRowsForParallelJoinBuild = minTableRowsForParallelJoinBuild; + options.vectorHasherMaxNumDistinct = joinBuildVectorHasherMaxNumDistinct; + options.abandonHashBuildDedupMinRows = abandonHashBuildDedupMinRows; + options.abandonHashBuildDedupMinPct = abandonHashBuildDedupMinPct; + options.bloomFilterPushdownMaxSize = bloomFilterPushdownSize; + + builder_ = std::make_unique<exec::JoinTableBuilder>(std::move(options)); + + // The filter columns are resolved on the Java side, hence there is no filter + // expression to analyze here. Review Comment: The PR description is currently the default template and doesn’t explain the refactor scope or how it was tested. Given this change swaps out core hash build logic for `JoinTableBuilder` and changes the Velox dependency branch, please fill in the PR description with (a) rationale/behavioral equivalence notes (especially around null-aware anti joins + filters), and (b) concrete test evidence (unit/integration tests or benchmark query set). -- 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]
