Copilot commented on code in PR #12793:
URL: https://github.com/apache/gluten/pull/12793#discussion_r3796825876
##########
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));
+ }
+
+ bool testInt64Range(int64_t /*min*/, int64_t /*max*/, bool /*hasNull*/)
const override {
+ return true;
+ }
+
+ std::unique_ptr<Filter> clone(std::optional<bool> nullAllowed) const
override {
+ return std::make_unique<SparkMightContain<kIsInt32>>(constantVector_,
nullAllowed.value_or(nullAllowed_), seed_);
+ }
+
+ bool testingEquals(const Filter& other) const override {
+ return dynamic_cast<const SparkMightContain<kIsInt32>*>(&other) != nullptr;
+ }
Review Comment:
`testingEquals` currently returns true for any filter of the same type,
ignoring `seed_` and the Bloom filter payload. This can make tests pass
incorrectly and can break any logic relying on `testingEquals` for semantic
equality. Compare at least `seed_`, `nullAllowed_`, and the underlying Bloom
filter bytes (or a stable fingerprint of them).
--
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]
