Copilot commented on code in PR #12793:
URL: https://github.com/apache/gluten/pull/12793#discussion_r3796825863
##########
cpp/velox/operators/functions/SparkExprToSubfieldFilterParser.cc:
##########
@@ -16,11 +16,83 @@
*/
#include "operators/functions/SparkExprToSubfieldFilterParser.h"
+#include "utils/Exception.h"
+#include "velox/common/base/BloomFilter.h"
+#include "velox/expression/Expr.h"
+#include "velox/functions/sparksql/XxHash64.h"
+#include "velox/vector/ComplexVector.h"
+
namespace gluten {
using namespace facebook::velox;
namespace {
+
+// Evaluates an expression as a constant. Returns nullptr if the expression is
+// not constant or evaluation fails. Errors are intentionally swallowed because
+// a non-evaluable expression simply means the filter cannot be pushed down.
+VectorPtr toConstant(const core::TypedExprPtr& expr,
core::ExpressionEvaluator* evaluator) {
+ auto exprSet = evaluator->compile(expr);
+ if (!exprSet->exprs()[0]->isConstantExpr()) {
+ return nullptr;
+ }
+ RowVector input(evaluator->pool(), ROW({}, {}), nullptr, 1,
std::vector<VectorPtr>{});
+ SelectivityVector rows(1);
+ VectorPtr result;
+ try {
+ evaluator->evaluate(exprSet.get(), rows, input, result);
+ } catch (const VeloxUserError& error) {
+ VLOG(1) << "Failed to evaluate constant expression for scan filter
pushdown: " << error.what();
+ return nullptr;
+ }
+ return result;
+}
+
+/// Subfield filter backed by Velox's BloomFilter from bloom_filter_agg /
might_contain.
+/// Values are hashed with Spark-compatible XXH64 using the seed extracted from
+/// xxhash64_with_seed, then re-hashed with folly hasher for bloom filter
bucket
+/// selection, matching bloom_filter_agg's insertion path.
+template <bool kIsInt32>
+class SparkMightContain final : public common::BigintValuesUsingBloomFilter {
+ public:
+ SparkMightContain(VectorPtr constantVector, bool nullAllowed, int64_t seed)
+ : common::BigintValuesUsingBloomFilter(0, nullAllowed),
constantVector_(std::move(constantVector)), seed_(seed) {
+ auto sv = constantVector_->as<SimpleVector<StringView>>()->valueAt(0);
+ view_ = std::make_unique<BloomFilterView>(sv.data());
+ }
+
+ bool testInt64(int64_t value) const override {
+ uint64_t hash;
+ if constexpr (kIsInt32) {
+ hash =
functions::sparksql::XxHash64::hashInt32(static_cast<int32_t>(value), seed_);
+ } else {
+ hash = functions::sparksql::XxHash64::hashInt64(value, seed_);
+ }
+ return view_->mayContain(folly::hasher<int64_t>()(hash));
Review Comment:
`hash` is `uint64_t` but is passed through `folly::hasher<int64_t>()(hash)`.
Converting an out-of-range `uint64_t` to `int64_t` is implementation-defined
and can also change the effective hash distribution, risking false negatives
(incorrectly filtering out rows). Use a hasher matching the unsigned type
(e.g., `folly::hasher<uint64_t>()`) or ensure the exact same signed/unsigned
representation is used as in `bloom_filter_agg` insertion.
--
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]
