From 98e86c9e2bd96e4a8af8aee56d6b364ca59e5603 Mon Sep 17 00:00:00 2001 From: Dmitry Chirkov Date: Sat, 14 Nov 2026 00:32:42 -0800 Subject: [PATCH 1/3] GH-XXXXX: [C++][Gandiva] Add tests for expression cache key collisions The tests cover Boolean AND/OR structure, string literal boundaries, IN value lists, the output field type and field nullability, and the compact forms of type parameters and floating point literals. Most of them fail on main because different expressions get the same cache key. The fix is in the next commit. --- cpp/src/gandiva/cache_test.cc | 243 ++++++++++++++++++ cpp/src/gandiva/tests/boolean_expr_test.cc | 37 +++ cpp/src/gandiva/tests/if_expr_test.cc | 31 +++ .../tests/projector_build_validation_test.cc | 16 ++ 4 files changed, 327 insertions(+) diff --git a/cpp/src/gandiva/cache_test.cc b/cpp/src/gandiva/cache_test.cc index d371db59dfc2..ff1078df9eb1 100644 --- a/cpp/src/gandiva/cache_test.cc +++ b/cpp/src/gandiva/cache_test.cc @@ -16,9 +16,20 @@ // under the License. #include "gandiva/cache.h" + +#include +#include +#include +#include +#include +#include + #include "arrow/testing/gtest_util.h" #include "arrow/util/io_util.h" #include "arrow/util/logging.h" +#include "gandiva/condition.h" +#include "gandiva/expression_cache_key.h" +#include "gandiva/tree_expr_builder.h" #include @@ -107,4 +118,236 @@ TEST(TestCache, TestGetCacheCapacityEnvVar) { } } +namespace { + +ExpressionCacheKey MakeKey(const NodePtr& node, const DataTypePtr& type) { + return ExpressionCacheKey( + arrow::schema({}), ConfigurationBuilder::DefaultConfiguration(), + {TreeExprBuilder::MakeExpression(node, arrow::field("out", type))}, + SelectionVector::MODE_NONE); +} + +} // namespace + +TEST(TestExpressionCacheKey, IfLiteralBoundaries) { + auto truth = TreeExprBuilder::MakeLiteral(true); + const std::string delimiter = "' } else { (const string) '"; + auto long_then = TreeExprBuilder::MakeStringLiteral("x" + delimiter + "y"); + auto short_then = TreeExprBuilder::MakeStringLiteral("x"); + auto short_else = TreeExprBuilder::MakeStringLiteral("z"); + auto long_else = TreeExprBuilder::MakeStringLiteral("y" + delimiter + "z"); + auto a = TreeExprBuilder::MakeIf(truth, long_then, short_else, arrow::utf8()); + auto b = TreeExprBuilder::MakeIf(truth, short_then, long_else, arrow::utf8()); + ASSERT_EQ(a->ToString(), b->ToString()); + EXPECT_NE(MakeKey(a, arrow::utf8()), MakeKey(b, arrow::utf8())); + EXPECT_EQ(MakeKey(a, arrow::utf8()), MakeKey(a, arrow::utf8())); +} + +TEST(TestExpressionCacheKey, OutputTypeAndAlias) { + auto schema = arrow::schema({}); + auto config = ConfigurationBuilder::DefaultConfiguration(); + auto node = TreeExprBuilder::MakeLiteral(int32_t{1}); + auto make_key = [&](const FieldPtr& result) { + return ExpressionCacheKey(schema, config, + {TreeExprBuilder::MakeExpression(node, result)}, + SelectionVector::MODE_NONE); + }; + auto key = make_key(arrow::field("first", arrow::int32())); + EXPECT_EQ(key, make_key(arrow::field("second", arrow::int32(), false))); + EXPECT_NE(key, make_key(arrow::field("first", arrow::int64()))); + auto expression = + TreeExprBuilder::MakeExpression(node, arrow::field("first", arrow::int32())); + EXPECT_NE(key, ExpressionCacheKey(schema, config, {expression}, + SelectionVector::MODE_UINT16)); + Condition condition(node); + EXPECT_NE(MakeKey(node, arrow::boolean()), + ExpressionCacheKey(schema, config, condition)); +} + +TEST(TestExpressionCacheKey, FloatingPointBitsAndNulls) { + auto check = [](const auto& first, const auto& second, const DataTypePtr& type) { + EXPECT_NE(MakeKey(TreeExprBuilder::MakeLiteral(first), type), + MakeKey(TreeExprBuilder::MakeLiteral(second), type)); + }; + check(0.0f, -0.0f, arrow::float32()); + check(0.0, -0.0, arrow::float64()); + check(1.0f, std::nextafter(1.0f, 2.0f), arrow::float32()); + check(1.0, std::nextafter(1.0, 2.0), arrow::float64()); + uint64_t bits_a = 0x7ff8000000000001ULL; + uint64_t bits_b = 0x7ff8000000000002ULL; + double nan_a; + double nan_b; + std::memcpy(&nan_a, &bits_a, sizeof(nan_a)); + std::memcpy(&nan_b, &bits_b, sizeof(nan_b)); + check(nan_a, nan_b, arrow::float64()); + EXPECT_NE(MakeKey(TreeExprBuilder::MakeNull(arrow::utf8()), arrow::utf8()), + MakeKey(TreeExprBuilder::MakeStringLiteral("null"), arrow::utf8())); + EXPECT_NE( + MakeKey(TreeExprBuilder::MakeStringLiteral(std::string("a\0b", 3)), arrow::utf8()), + MakeKey(TreeExprBuilder::MakeStringLiteral("a"), arrow::utf8())); +} + +TEST(TestExpressionCacheKey, InSetsAreDelimitedAndOrdered) { + auto node = TreeExprBuilder::MakeStringLiteral("a"); + auto one = TreeExprBuilder::MakeInExpressionString(node, {"a, b"}); + auto two = TreeExprBuilder::MakeInExpressionString(node, {"a", "b"}); + ASSERT_NE(MakeKey(one, arrow::boolean()), MakeKey(two, arrow::boolean())); + EXPECT_EQ(MakeKey(two, arrow::boolean()), + MakeKey(TreeExprBuilder::MakeInExpressionString(node, {"b", "a"}), + arrow::boolean())); + auto float_node = TreeExprBuilder::MakeLiteral(1.0f); + EXPECT_NE(MakeKey(TreeExprBuilder::MakeInExpressionFloat(float_node, {1.0f}), + arrow::boolean()), + MakeKey(TreeExprBuilder::MakeInExpressionFloat(float_node, + {std::nextafter(1.0f, 2.0f)}), + arrow::boolean())); + auto double_node = TreeExprBuilder::MakeLiteral(1.0); + EXPECT_NE(MakeKey(TreeExprBuilder::MakeInExpressionDouble(double_node, {1.0}), + arrow::boolean()), + MakeKey(TreeExprBuilder::MakeInExpressionDouble(double_node, + {std::nextafter(1.0, 2.0)}), + arrow::boolean())); + auto binary_node = TreeExprBuilder::MakeBinaryLiteral("a"); + EXPECT_NE(MakeKey(TreeExprBuilder::MakeInExpressionBinary(binary_node, {"a"}), + arrow::boolean()), + MakeKey(TreeExprBuilder::MakeInExpressionString(binary_node, {"a"}), + arrow::boolean())); + std::unordered_set decimals_a = {DecimalScalar128("123", 10, 2)}; + std::unordered_set decimals_b = {DecimalScalar128("124", 10, 2)}; + auto decimal_node = TreeExprBuilder::MakeDecimalLiteral(DecimalScalar128("123", 10, 2)); + EXPECT_NE(MakeKey(TreeExprBuilder::MakeInExpressionDecimal(decimal_node, decimals_a), + arrow::boolean()), + MakeKey(TreeExprBuilder::MakeInExpressionDecimal(decimal_node, decimals_b), + arrow::boolean())); +} + +TEST(TestExpressionCacheKey, BooleanStructure) { + auto a = TreeExprBuilder::MakeField(arrow::field("a", arrow::boolean())); + auto b = TreeExprBuilder::MakeField(arrow::field("b", arrow::boolean())); + auto c = TreeExprBuilder::MakeField(arrow::field("c", arrow::boolean())); + auto first = TreeExprBuilder::MakeAnd({a, TreeExprBuilder::MakeOr({b, c})}); + auto second = TreeExprBuilder::MakeOr({TreeExprBuilder::MakeAnd({a, b}), c}); + ASSERT_EQ(first->ToString(), second->ToString()); + EXPECT_NE(MakeKey(first, arrow::boolean()), MakeKey(second, arrow::boolean())); +} + +namespace { + +void ExpectAllDistinct(const std::vector& keys) { + for (size_t i = 0; i < keys.size(); ++i) { + for (size_t j = i + 1; j < keys.size(); ++j) { + EXPECT_NE(keys[i], keys[j]) << "keys " << i << " and " << j << " are equal"; + } + } +} + +NodePtr Function(const std::string& name, const NodeVector& children, + const DataTypePtr& type) { + return TreeExprBuilder::MakeFunction(name, children, type); +} + +NodePtr Field(const std::string& name, const DataTypePtr& type, bool nullable = true) { + return TreeExprBuilder::MakeField(arrow::field(name, type, nullable)); +} + +} // namespace + +TEST(TestExpressionCacheKey, TypeParametersAreKept) { + // Literals of different types can share a holder, so only the type tells them apart. + auto int_literal = [](const DataTypePtr& type) -> NodePtr { + return std::make_shared(type, LiteralHolder(int32_t{1}), false); + }; + auto long_literal = [](const DataTypePtr& type) -> NodePtr { + return std::make_shared(type, LiteralHolder(int64_t{1}), false); + }; + auto decimal_literal = [](const DataTypePtr& type) -> NodePtr { + return std::make_shared( + type, LiteralHolder(DecimalScalar128("1", 10, 2)), false); + }; + + std::vector keys; + for (const auto& type : + {arrow::int32(), arrow::date32(), arrow::time32(arrow::TimeUnit::SECOND), + arrow::time32(arrow::TimeUnit::MILLI)}) { + keys.push_back(MakeKey(int_literal(type), type)); + } + for (const auto& type : + {arrow::int64(), arrow::date64(), arrow::timestamp(arrow::TimeUnit::SECOND), + arrow::timestamp(arrow::TimeUnit::MILLI), + arrow::timestamp(arrow::TimeUnit::MICRO), + arrow::timestamp(arrow::TimeUnit::MILLI, "UTC"), + arrow::timestamp(arrow::TimeUnit::MILLI, "Asia/Tokyo"), + arrow::time64(arrow::TimeUnit::MICRO), arrow::time64(arrow::TimeUnit::NANO)}) { + keys.push_back(MakeKey(long_literal(type), type)); + } + for (const auto& type : {arrow::decimal128(10, 2), arrow::decimal128(10, 3), + arrow::decimal128(11, 2), arrow::decimal128(38, 2)}) { + keys.push_back(MakeKey(decimal_literal(type), type)); + } + ExpectAllDistinct(keys); + + // The same type built twice is the same type. + EXPECT_EQ(MakeKey(decimal_literal(arrow::decimal128(10, 2)), arrow::decimal128(10, 2)), + MakeKey(decimal_literal(arrow::decimal128(10, 2)), arrow::decimal128(10, 2))); + EXPECT_EQ(MakeKey(long_literal(arrow::timestamp(arrow::TimeUnit::MILLI, "UTC")), + arrow::timestamp(arrow::TimeUnit::MILLI, "UTC")), + MakeKey(long_literal(arrow::timestamp(arrow::TimeUnit::MILLI, "UTC")), + arrow::timestamp(arrow::TimeUnit::MILLI, "UTC"))); + + // A decimal output type is part of the key even when the root has the same text. + auto node = decimal_literal(arrow::decimal128(10, 2)); + EXPECT_NE(MakeKey(node, arrow::decimal128(10, 2)), + MakeKey(node, arrow::decimal128(10, 3))); +} + +TEST(TestExpressionCacheKey, RepeatedNamesAndTypesStayDistinct) { + // Repeated names and types are written as references. These pairs differ only in + // where a name or type repeats, or in the order the types first appear. + auto i32 = arrow::int32(); + auto i64 = arrow::int64(); + std::vector keys; + auto add = [&](const NodePtr& node) { keys.push_back(MakeKey(node, i64)); }; + + add(Function("f", {Field("a", i32), Field("a", i32)}, i64)); + add(Function("f", {Field("a", i32), Field("b", i32)}, i64)); + add(Function("f", {Field("a", i32), Field("a", i64)}, i64)); + add(Function("f", {Field("a", i32), Field("b", i64), Field("c", i32)}, i64)); + add(Function("f", {Field("a", i32), Field("b", i64), Field("c", i64)}, i64)); + add(Function("f", {Field("a", i64), Field("b", i32), Field("c", i64)}, i64)); + add(Function("f", {Field("a", i32), Field("b", i32), Field("c", i64)}, i64)); + // A function name and a field name share one name table. + add(Function("x", {Field("x", i32)}, i64)); + add(Function("x", {Field("y", i32)}, i64)); + add(Function("y", {Field("x", i32)}, i64)); + add(Function("x", {Function("x", {Field("x", i32)}, i64)}, i64)); + // Nullability and function result types. + add(Function("f", {Field("a", i32, false)}, i64)); + add(Function("f", {Field("a", i32, true)}, i64)); + add(Function("f", {Field("a", i32, true)}, i32)); + ExpectAllDistinct(keys); + + // Different field objects with the same content give the same key. + EXPECT_EQ(MakeKey(Function("f", {Field("a", i32), Field("a", i32)}, i64), i64), + MakeKey(Function("f", {Field("a", i32), Field("a", i32)}, i64), i64)); +} + +TEST(TestExpressionCacheKey, LikeThreadUniquifier) { + auto literal = TreeExprBuilder::MakeStringLiteral("text like("); + auto like = TreeExprBuilder::MakeFunction( + "like", {literal, TreeExprBuilder::MakeStringLiteral("%")}, arrow::boolean()); + auto nested = TreeExprBuilder::MakeIf(like, literal, literal, arrow::utf8()); + using ThreadKeys = std::pair>; + auto make_keys = [&]() -> ThreadKeys { + return {std::hash()(std::this_thread::get_id()) % 16, + {MakeKey(nested, arrow::utf8()), MakeKey(literal, arrow::utf8())}}; + }; + auto main_keys = make_keys(); + for (int i = 0; i < 32; ++i) { + auto other = std::async(std::launch::async, make_keys).get(); + EXPECT_EQ(main_keys.first == other.first, + main_keys.second.first == other.second.first); + EXPECT_EQ(main_keys.second.second, other.second.second); + } +} + } // namespace gandiva diff --git a/cpp/src/gandiva/tests/boolean_expr_test.cc b/cpp/src/gandiva/tests/boolean_expr_test.cc index a54f7622ba20..0bb74145e384 100644 --- a/cpp/src/gandiva/tests/boolean_expr_test.cc +++ b/cpp/src/gandiva/tests/boolean_expr_test.cc @@ -434,4 +434,41 @@ TEST_F(TestBooleanExpr, IfInsideBooleanAnd) { EXPECT_ARROW_ARRAY_EQUALS(exp, outputs.at(0)); } +TEST_F(TestBooleanExpr, CacheKeepsAndOrStructure) { + auto field_a = field("cache_key_a", boolean()); + auto field_b = field("cache_key_b", boolean()); + auto field_c = field("cache_key_c", boolean()); + auto schema = arrow::schema({field_a, field_b, field_c}); + auto field_result = field("res", boolean()); + + auto node_a = TreeExprBuilder::MakeField(field_a); + auto node_b = TreeExprBuilder::MakeField(field_b); + auto node_c = TreeExprBuilder::MakeField(field_c); + // a && (b || c) and (a && b) || c print as the same text. + auto and_or = + TreeExprBuilder::MakeAnd({node_a, TreeExprBuilder::MakeOr({node_b, node_c})}); + auto or_and = + TreeExprBuilder::MakeOr({TreeExprBuilder::MakeAnd({node_a, node_b}), node_c}); + ASSERT_EQ(and_or->ToString(), or_and->ToString()); + + // a = false, b = false, c = true + auto in_a = MakeArrowArrayBool({false}, {true}); + auto in_b = MakeArrowArrayBool({false}, {true}); + auto in_c = MakeArrowArrayBool({true}, {true}); + auto batch = arrow::RecordBatch::Make(schema, 1, {in_a, in_b, in_c}); + + auto configuration = TestConfiguration(); + auto evaluate = [&](const NodePtr& node, + const std::shared_ptr& expected) { + std::shared_ptr projector; + auto expr = TreeExprBuilder::MakeExpression(node, field_result); + ASSERT_OK(Projector::Make(schema, {expr}, configuration, &projector)); + arrow::ArrayVector outputs; + ASSERT_OK(projector->Evaluate(*batch, pool_, &outputs)); + EXPECT_ARROW_ARRAY_EQUALS(expected, outputs.at(0)); + }; + evaluate(and_or, MakeArrowArrayBool({false}, {true})); + evaluate(or_and, MakeArrowArrayBool({true}, {true})); +} + } // namespace gandiva diff --git a/cpp/src/gandiva/tests/if_expr_test.cc b/cpp/src/gandiva/tests/if_expr_test.cc index 54b6d43b4df1..542968556bde 100644 --- a/cpp/src/gandiva/tests/if_expr_test.cc +++ b/cpp/src/gandiva/tests/if_expr_test.cc @@ -375,4 +375,35 @@ TEST_F(TestIfExpr, TestBigNested) { EXPECT_ARROW_ARRAY_EQUALS(exp, outputs.at(0)); } +TEST_F(TestIfExpr, CacheKeepsStringLiteralBoundaries) { + auto field_condition = field("cache_key_condition", boolean()); + auto schema = arrow::schema({field_condition}); + auto field_result = field("res", arrow::utf8()); + auto condition = TreeExprBuilder::MakeField(field_condition); + + // Both expressions print as the same text, but the literals split in different places. + const std::string delimiter = "' } else { (const string) '"; + auto first = TreeExprBuilder::MakeIf( + condition, TreeExprBuilder::MakeStringLiteral("x" + delimiter + "y"), + TreeExprBuilder::MakeStringLiteral("z"), arrow::utf8()); + auto second = TreeExprBuilder::MakeIf( + condition, TreeExprBuilder::MakeStringLiteral("x"), + TreeExprBuilder::MakeStringLiteral("y" + delimiter + "z"), arrow::utf8()); + ASSERT_EQ(first->ToString(), second->ToString()); + + auto batch = arrow::RecordBatch::Make(schema, 1, {MakeArrowArrayBool({true}, {true})}); + auto configuration = TestConfiguration(); + auto evaluate = [&](const NodePtr& node, + const std::shared_ptr& expected) { + std::shared_ptr projector; + auto expr = TreeExprBuilder::MakeExpression(node, field_result); + ASSERT_OK(Projector::Make(schema, {expr}, configuration, &projector)); + arrow::ArrayVector outputs; + ASSERT_OK(projector->Evaluate(*batch, pool_, &outputs)); + EXPECT_ARROW_ARRAY_EQUALS(expected, outputs.at(0)); + }; + evaluate(first, MakeArrowArrayUtf8({"x" + delimiter + "y"}, {true})); + evaluate(second, MakeArrowArrayUtf8({"x"}, {true})); +} + } // namespace gandiva diff --git a/cpp/src/gandiva/tests/projector_build_validation_test.cc b/cpp/src/gandiva/tests/projector_build_validation_test.cc index 1ed4c77a074a..a64380615c34 100644 --- a/cpp/src/gandiva/tests/projector_build_validation_test.cc +++ b/cpp/src/gandiva/tests/projector_build_validation_test.cc @@ -284,4 +284,20 @@ TEST_F(ValidationTestProjector, TestAndBooleanArgType) { EXPECT_TRUE(status.IsExpressionValidationError()); } +TEST_F(ValidationTestProjector, TestCachedOutputTypeValidation) { + auto schema = arrow::schema({field("cached_output_type", int32())}); + auto configuration = TestConfiguration(); + auto node = TreeExprBuilder::MakeLiteral(int32_t{1}); + + // Same expression tree, compiled once with its own output type. + std::shared_ptr projector; + auto valid = TreeExprBuilder::MakeExpression(node, field("res", int32())); + ASSERT_OK(Projector::Make(schema, {valid}, configuration, &projector)); + + // A different output type must not reuse the cached code and skip validation. + auto invalid = TreeExprBuilder::MakeExpression(node, field("res", arrow::int64())); + auto status = Projector::Make(schema, {invalid}, configuration, &projector); + EXPECT_TRUE(status.IsExpressionValidationError()) << status.ToString(); +} + } // namespace gandiva From 16a04c1bf01d0c32042e8e32b9a34fd51c64655c Mon Sep 17 00:00:00 2001 From: Dmitry Chirkov Date: Sat, 14 Nov 2026 00:32:45 -0800 Subject: [PATCH 2/3] GH-XXXXX: [C++][Gandiva] Build the expression cache key from the expression tree The cache key was built from Expression::ToString(), so different expressions could print the same text and share one compiled entry. Build the key from the expression tree and the output field type instead, with length-prefixed tokens and child counts. --- cpp/src/gandiva/expression_cache_key.h | 293 ++++++++++++++++++++++++- 1 file changed, 287 insertions(+), 6 deletions(-) diff --git a/cpp/src/gandiva/expression_cache_key.h b/cpp/src/gandiva/expression_cache_key.h index e7522042a7d4..052522d24ca1 100644 --- a/cpp/src/gandiva/expression_cache_key.h +++ b/cpp/src/gandiva/expression_cache_key.h @@ -19,17 +19,295 @@ #include +#include +#include #include +#include +#include +#include #include "arrow/util/hash_util.h" #include "gandiva/arrow.h" #include "gandiva/configuration.h" #include "gandiva/expression.h" +#include "gandiva/node.h" #include "gandiva/selection_vector.h" #include "gandiva/visibility.h" namespace gandiva { +namespace internal { + +// Debug strings are ambiguous when literal values or field names contain expression +// syntax. Encode the tree in a compact binary form that has one possible parse: every +// node starts with a one byte tag, variable size tokens carry their length, and child +// lists carry their count. Keys are process-local, so values are stored as raw bytes. +// +// Repeated types and names are written in full once per expression and as a short +// reference after that. A reference points at the n-th definition in the same key, so +// it stays unambiguous. Types are compared by their fingerprint, not by pointer, so two +// separately built `decimal128(10, 2)` still give the same key. +// +// The return type of a node is written only when the node does not already fix it: +// a field node has the type of its field, and boolean and IN nodes are always boolean. +// This runs before validation, and a cache hit skips validation, so the key has to tell +// apart every expression that validation could tell apart. Null nodes and types must +// be safe. +class ExpressionKeyVisitor : public NodeVisitor { + public: + std::string MakeKey(const Expression* expression, const char* kind) { + key_.clear(); + type_by_pointer_.clear(); + type_by_definition_.clear(); + name_index_.clear(); + has_like_ = false; + + AppendBytes(&key_, kind); + if (!expression) { + key_ += kNullExpression; + return key_; + } + key_ += kExpression; + if (expression->result()) { + key_ += kResult; + AppendType(expression->result()->type()); + } else { + key_ += kNullResult; + } + AppendNode(expression->root()); + return key_; + } + + bool has_like() const { return has_like_; } + + Status Visit(const FieldNode& node) override { + key_ += kFieldNode; + AppendField(node.field()); + return Status::OK(); + } + + Status Visit(const FunctionNode& node) override { + key_ += kFunctionNode; + AppendType(node.return_type()); + AppendName(node.descriptor()->name()); + has_like_ |= node.descriptor()->name() == "like"; + AppendNodes(node.children()); + return Status::OK(); + } + + Status Visit(const IfNode& node) override { + key_ += kIfNode; + AppendType(node.return_type()); + AppendNode(node.condition()); + AppendNode(node.then_node()); + AppendNode(node.else_node()); + return Status::OK(); + } + + Status Visit(const LiteralNode& node) override { + key_ += kLiteralNode; + AppendType(node.return_type()); + key_ += node.is_null() ? kNullLiteral : kValueLiteral; + // The variant index fixes how many bytes follow, so values need no length. + key_ += static_cast(node.holder().index()); + std::visit([this](const auto& value) { AppendValue(&key_, value); }, node.holder()); + return Status::OK(); + } + + Status Visit(const BooleanNode& node) override { + key_ += kBooleanNode; + key_ += static_cast(node.expr_type()); + AppendNodes(node.children()); + return Status::OK(); + } + + Status Visit(const InExpressionNode& node) override { + return AppendIn(node, 'a'); + } + Status Visit(const InExpressionNode& node) override { + return AppendIn(node, 'b'); + } + Status Visit(const InExpressionNode& node) override { + return AppendIn(node, 'c'); + } + Status Visit(const InExpressionNode& node) override { + return AppendIn(node, 'd'); + } + Status Visit(const InExpressionNode& node) override { + return AppendIn(node, 's'); + } + + Status Visit(const InExpressionNode& node) override { + key_ += kInNode; + key_ += 'D'; + AppendVarint(&key_, static_cast(node.get_precision())); + AppendVarint(&key_, static_cast(node.get_scale())); + return AppendInValues(node); + } + + private: + static constexpr char kNullExpression = 'X'; + static constexpr char kExpression = 'E'; + static constexpr char kResult = 'R'; + static constexpr char kNullResult = 'r'; + static constexpr char kNullNode = 'z'; + static constexpr char kFieldNode = 'f'; + static constexpr char kFunctionNode = 'c'; + static constexpr char kIfNode = 'i'; + static constexpr char kLiteralNode = 'l'; + static constexpr char kBooleanNode = 'b'; + static constexpr char kInNode = 'I'; + static constexpr char kNullLiteral = 'n'; + static constexpr char kValueLiteral = 'v'; + static constexpr char kNullType = 'N'; + static constexpr char kTypeDefinition = 'T'; + static constexpr char kTypeReference = 't'; + static constexpr char kNullField = 'Z'; + static constexpr char kField = 'F'; + static constexpr char kNullable = '1'; + static constexpr char kNonNullable = '0'; + static constexpr char kNameDefinition = 'M'; + static constexpr char kNameReference = 'm'; + + static void AppendVarint(std::string* out, uint64_t value) { + while (value >= 0x80) { + out->push_back(static_cast(value | 0x80)); + value >>= 7; + } + out->push_back(static_cast(value)); + } + + static void AppendBytes(std::string* out, const std::string& value) { + AppendVarint(out, value.size()); + out->append(value); + } + + template + static void AppendValue(std::string* out, const T& value) { + static_assert(std::is_arithmetic_v); + // Preserve exact numeric bytes, including float NaN payloads and signed zero, + // without decimal formatting or rounding. + out->append(reinterpret_cast(&value), sizeof(value)); + } + + static void AppendValue(std::string* out, const std::string& value) { + AppendBytes(out, value); + } + + static void AppendValue(std::string* out, const DecimalScalar128& value) { + AppendValue(out, value.value().high_bits()); + AppendValue(out, value.value().low_bits()); + AppendValue(out, value.precision()); + AppendValue(out, value.scale()); + } + + void AppendType(const DataTypePtr& type) { + if (!type) { + key_ += kNullType; + return; + } + auto pointer = type_by_pointer_.try_emplace(type.get(), 0); + if (!pointer.second) { + key_ += kTypeReference; + AppendVarint(&key_, pointer.first->second); + return; + } + + // The fingerprint holds the type parameters: precision and scale, time unit and + // time zone. Types without a fingerprint fall back to their full description. + std::string definition; + AppendVarint(&definition, static_cast(type->id())); + const std::string& fingerprint = type->fingerprint(); + AppendBytes(&definition, fingerprint); + if (fingerprint.empty()) { + AppendBytes(&definition, type->ToString()); + } + + auto known = type_by_definition_.try_emplace(definition, type_by_definition_.size()); + pointer.first->second = known.first->second; + if (known.second) { + key_ += kTypeDefinition; + key_ += definition; + } else { + key_ += kTypeReference; + AppendVarint(&key_, known.first->second); + } + } + + void AppendName(const std::string& name) { + auto it = name_index_.find(name); + if (it != name_index_.end()) { + key_ += kNameReference; + AppendVarint(&key_, it->second); + return; + } + name_index_.emplace(name, name_index_.size()); + key_ += kNameDefinition; + AppendBytes(&key_, name); + } + + void AppendField(const FieldPtr& field) { + if (!field) { + key_ += kNullField; + return; + } + key_ += kField; + AppendName(field->name()); + AppendType(field->type()); + key_ += field->nullable() ? kNullable : kNonNullable; + } + + void AppendNode(const NodePtr& node) { + if (!node) { + key_ += kNullNode; + return; + } + // All visits in this encoder return OK; validation is a separate pass. + ARROW_UNUSED(node->Accept(*this)); + } + + void AppendNodes(const NodeVector& nodes) { + AppendVarint(&key_, nodes.size()); + for (const auto& node : nodes) { + AppendNode(node); + } + } + + template + Status AppendIn(const InExpressionNode& node, char kind) { + key_ += kInNode; + key_ += kind; + AppendType(node.type()); + return AppendInValues(node); + } + + template + Status AppendInValues(const InExpressionNode& node) { + AppendNode(node.eval_expr()); + // The value set is unordered, so write the values in byte order. + std::vector values; + values.reserve(node.values().size()); + for (const auto& value : node.values()) { + values.emplace_back(); + AppendValue(&values.back(), value); + } + std::sort(values.begin(), values.end()); + AppendVarint(&key_, values.size()); + for (const auto& value : values) { + key_ += value; + } + return Status::OK(); + } + + std::string key_; + bool has_like_ = false; + std::unordered_map type_by_pointer_; + std::unordered_map type_by_definition_; + std::unordered_map name_index_; +}; + +} // namespace internal + class ExpressionCacheKey { public: ExpressionCacheKey(SchemaPtr schema, std::shared_ptr configuration, @@ -38,10 +316,11 @@ class ExpressionCacheKey { static const int kSeedValue = 4; size_t result = kSeedValue; for (auto& expr : expression_vector) { - std::string expr_as_string = expr->ToString(); + internal::ExpressionKeyVisitor visitor; + std::string expr_as_string = visitor.MakeKey(expr.get(), "projector"); expressions_as_strings_.push_back(expr_as_string); arrow::internal::hash_combine(result, expr_as_string); - UpdateUniquifier(expr_as_string); + UpdateUniquifier(visitor.has_like()); } arrow::internal::hash_combine(result, static_cast(mode)); arrow::internal::hash_combine(result, configuration->Hash()); @@ -58,20 +337,22 @@ class ExpressionCacheKey { configuration_(configuration) { static const int kSeedValue = 4; size_t result = kSeedValue; - expressions_as_strings_.push_back(expression.ToString()); - UpdateUniquifier(expression.ToString()); + internal::ExpressionKeyVisitor visitor; + expressions_as_strings_.push_back(visitor.MakeKey(&expression, "filter")); + UpdateUniquifier(visitor.has_like()); + arrow::internal::hash_combine(result, expressions_as_strings_.back()); arrow::internal::hash_combine(result, configuration->Hash()); arrow::internal::hash_combine(result, schema_->ToString()); arrow::internal::hash_combine(result, uniquifier_); hash_code_ = result; } - void UpdateUniquifier(const std::string& expr) { + void UpdateUniquifier(bool has_like) { if (uniquifier_ == 0) { // caching of expressions with re2 patterns causes lock contention. So, use // multiple instances to reduce contention. - if (expr.find(" like(") != std::string::npos) { + if (has_like) { uniquifier_ = std::hash()(std::this_thread::get_id()) % 16; } } From cb1781d33e7f099780949c02079f549dc3ad1b45 Mon Sep 17 00:00:00 2001 From: Dmitry Chirkov Date: Sat, 14 Nov 2026 11:15:14 -0800 Subject: [PATCH 3/3] GH-XXXXX: [C++][Gandiva] Add regression tests for cache key nullability and null nodes Add tests for projectors and filters built from two schemas that differ only in nullability, for a FieldNode whose nullability differs from the schema field, and for null child nodes in the cache key. Two schemas that differ only in nullability already get separate cache entries. A cached FieldNode with the wrong nullability was accepted on the old key and is rejected now. A null child node crashed the old key. --- cpp/src/gandiva/cache_test.cc | 68 +++++++++++++++++++ cpp/src/gandiva/tests/filter_test.cc | 61 +++++++++++++++++ .../tests/projector_build_validation_test.cc | 47 +++++++++++++ cpp/src/gandiva/tests/projector_test.cc | 67 ++++++++++++++++++ 4 files changed, 243 insertions(+) diff --git a/cpp/src/gandiva/cache_test.cc b/cpp/src/gandiva/cache_test.cc index ff1078df9eb1..6e64ca518a17 100644 --- a/cpp/src/gandiva/cache_test.cc +++ b/cpp/src/gandiva/cache_test.cc @@ -331,6 +331,74 @@ TEST(TestExpressionCacheKey, RepeatedNamesAndTypesStayDistinct) { MakeKey(Function("f", {Field("a", i32), Field("a", i32)}, i64), i64)); } +TEST(TestExpressionCacheKey, NullChildNodes) { + // The builder refuses null children, but some node constructors do not. The key is + // built before validation, so it must handle them and leave the error to the + // validator. FunctionNode is not covered: its constructor reads every child. + auto i32 = arrow::int32(); + auto a = Field("a", i32); + auto b = Field("b", i32); + auto boolean = arrow::boolean(); + const NodePtr null_node; + std::vector keys; + auto add = [&](const NodePtr& node, const DataTypePtr& type) { + keys.push_back(MakeKey(node, type)); + }; + + add(std::make_shared(null_node, a, b, i32), i32); + add(std::make_shared(a, null_node, b, i32), i32); + add(std::make_shared(a, b, null_node, i32), i32); + add(std::make_shared(a, b, b, i32), i32); + add(std::make_shared(BooleanNode::AND, NodeVector{null_node, a}), boolean); + add(std::make_shared(BooleanNode::AND, NodeVector{a, null_node}), boolean); + add(std::make_shared(BooleanNode::OR, NodeVector{null_node, a}), boolean); + add(std::make_shared>(null_node, + std::unordered_set{1}), + boolean); + add(std::make_shared>(null_node, + std::unordered_set{2}), + boolean); + add(std::make_shared>(a, std::unordered_set{1}), + boolean); + add(std::make_shared>(null_node, + std::unordered_set{1}), + boolean); + add(std::make_shared>(null_node, std::unordered_set{1}), + boolean); + add(std::make_shared>(null_node, + std::unordered_set{1}), + boolean); + add(std::make_shared>( + null_node, std::unordered_set{"a"}), + boolean); + std::unordered_set decimals{DecimalScalar128("1", 10, 2)}; + add(std::make_shared>(null_node, decimals, 10, 2), + boolean); + add(null_node, i32); + ExpectAllDistinct(keys); + + // The same tree built twice gives the same key. + EXPECT_EQ(MakeKey(std::make_shared(null_node, a, b, i32), i32), + MakeKey(std::make_shared(null_node, a, b, i32), i32)); + EXPECT_EQ(MakeKey(std::make_shared>( + null_node, std::unordered_set{"a"}), + boolean), + MakeKey(std::make_shared>( + null_node, std::unordered_set{"a"}), + boolean)); + + // A null expression and a null result field are also keyed without a crash. + auto key_of = [](const ExpressionPtr& expression) { + return ExpressionCacheKey(arrow::schema({}), + ConfigurationBuilder::DefaultConfiguration(), {expression}, + SelectionVector::MODE_NONE); + }; + auto without_result = key_of(std::make_shared(a, nullptr)); + auto without_expression = key_of(nullptr); + EXPECT_NE(without_result, without_expression); + EXPECT_NE(without_result, MakeKey(a, i32)); +} + TEST(TestExpressionCacheKey, LikeThreadUniquifier) { auto literal = TreeExprBuilder::MakeStringLiteral("text like("); auto like = TreeExprBuilder::MakeFunction( diff --git a/cpp/src/gandiva/tests/filter_test.cc b/cpp/src/gandiva/tests/filter_test.cc index 749000aa0cf2..68e066e390ae 100644 --- a/cpp/src/gandiva/tests/filter_test.cc +++ b/cpp/src/gandiva/tests/filter_test.cc @@ -449,4 +449,65 @@ TEST_F(TestFilter, TestLike) { EXPECT_ARROW_ARRAY_EQUALS(exp, selection_vector->ToArray()); } +// Two schemas that differ only in the nullability of a field are different cache +// entries, and a batch must match the schema the filter was built with. +TEST_F(TestFilter, TestCacheSchemasDifferingInNullability) { + auto nullable_field = field("filter_cache_nullability_f0", int32(), true); + auto required_field = field("filter_cache_nullability_f0", int32(), false); + auto nullable_schema = arrow::schema({nullable_field}); + auto required_schema = arrow::schema({required_field}); + + // The same condition on each schema's own field: f0 < 3. + auto make_condition = [](const FieldPtr& input) { + return TreeExprBuilder::MakeCondition(TreeExprBuilder::MakeFunction( + "less_than", {TreeExprBuilder::MakeField(input), TreeExprBuilder::MakeLiteral(3)}, + boolean())); + }; + auto configuration = TestConfiguration(); + + std::shared_ptr nullable_filter; + ASSERT_OK(Filter::Make(nullable_schema, make_condition(nullable_field), configuration, + &nullable_filter)); + EXPECT_FALSE(nullable_filter->GetBuiltFromCache()); + std::shared_ptr required_filter; + ASSERT_OK(Filter::Make(required_schema, make_condition(required_field), configuration, + &required_filter)); + EXPECT_FALSE(required_filter->GetBuiltFromCache()); + + std::shared_ptr nullable_again; + ASSERT_OK(Filter::Make(nullable_schema, make_condition(nullable_field), configuration, + &nullable_again)); + EXPECT_TRUE(nullable_again->GetBuiltFromCache()); + std::shared_ptr required_again; + ASSERT_OK(Filter::Make(required_schema, make_condition(required_field), configuration, + &required_again)); + EXPECT_TRUE(required_again->GetBuiltFromCache()); + + // Each filter selects the right rows on a batch of its own schema. Row 1 is null. + auto nullable_batch = arrow::RecordBatch::Make( + nullable_schema, 4, {MakeArrowArrayInt32({1, 2, 3, 4}, {true, false, true, true})}); + auto required_batch = + arrow::RecordBatch::Make(required_schema, 4, {MakeArrowArrayInt32({1, 2, 3, 4})}); + for (const auto& filter : {nullable_filter, nullable_again}) { + std::shared_ptr selection_vector; + ASSERT_OK(SelectionVector::MakeInt16(4, pool_, &selection_vector)); + ASSERT_OK(filter->Evaluate(*nullable_batch, selection_vector)); + EXPECT_ARROW_ARRAY_EQUALS(MakeArrowArrayUint16({0}), selection_vector->ToArray()); + } + for (const auto& filter : {required_filter, required_again}) { + std::shared_ptr selection_vector; + ASSERT_OK(SelectionVector::MakeInt16(4, pool_, &selection_vector)); + ASSERT_OK(filter->Evaluate(*required_batch, selection_vector)); + EXPECT_ARROW_ARRAY_EQUALS(MakeArrowArrayUint16({0, 1}), selection_vector->ToArray()); + } + + // A batch of the other schema is rejected, in both directions. + std::shared_ptr selection_vector; + ASSERT_OK(SelectionVector::MakeInt16(4, pool_, &selection_vector)); + auto status = nullable_filter->Evaluate(*required_batch, selection_vector); + EXPECT_TRUE(status.IsInvalid()) << status.ToString(); + status = required_filter->Evaluate(*nullable_batch, selection_vector); + EXPECT_TRUE(status.IsInvalid()) << status.ToString(); +} + } // namespace gandiva diff --git a/cpp/src/gandiva/tests/projector_build_validation_test.cc b/cpp/src/gandiva/tests/projector_build_validation_test.cc index a64380615c34..f8c4cd6428ff 100644 --- a/cpp/src/gandiva/tests/projector_build_validation_test.cc +++ b/cpp/src/gandiva/tests/projector_build_validation_test.cc @@ -300,4 +300,51 @@ TEST_F(ValidationTestProjector, TestCachedOutputTypeValidation) { EXPECT_TRUE(status.IsExpressionValidationError()) << status.ToString(); } +// Validation compares a field node with the schema field, including nullability. The +// key has to keep that difference, or a cache hit skips the validation error. +TEST_F(ValidationTestProjector, TestCachedFieldNullabilityValidation) { + auto configuration = TestConfiguration(); + // A unique field name, so that no other test seeds the cache for these expressions. + auto make_expression = [](const FieldPtr& node_field) { + auto node = TreeExprBuilder::MakeFunction( + "add", {TreeExprBuilder::MakeField(node_field), TreeExprBuilder::MakeLiteral(1)}, + int32()); + return TreeExprBuilder::MakeExpression(node, field("res", int32())); + }; + + // Cold: nothing is cached yet, so validation runs and rejects the mismatch. + auto cold_schema = arrow::schema({field("cold_nullability_field", int32(), true)}); + std::shared_ptr projector; + auto status = Projector::Make( + cold_schema, {make_expression(field("cold_nullability_field", int32(), false))}, + configuration, &projector); + EXPECT_TRUE(status.IsExpressionValidationError()) << status.ToString(); + + // Warm: the matching expression is compiled and cached first. + auto nullable_field = field("warm_nullability_field", int32(), true); + auto required_field = field("warm_nullability_field", int32(), false); + auto schema = arrow::schema({nullable_field}); + ASSERT_OK(Projector::Make(schema, {make_expression(nullable_field)}, configuration, + &projector)); + + // The same schema with a field node of the other nullability must still be rejected. + // Ask twice, so a hit on a cached entry is possible on both calls. + for (int i = 0; i < 2; ++i) { + status = Projector::Make(schema, {make_expression(required_field)}, configuration, + &projector); + EXPECT_TRUE(status.IsExpressionValidationError()) + << "call " << i << ": " << status.ToString(); + } + + // The matching expression still builds, and the other direction is rejected too. + ASSERT_OK(Projector::Make(schema, {make_expression(nullable_field)}, configuration, + &projector)); + auto required_schema = arrow::schema({required_field}); + ASSERT_OK(Projector::Make(required_schema, {make_expression(required_field)}, + configuration, &projector)); + status = Projector::Make(required_schema, {make_expression(nullable_field)}, + configuration, &projector); + EXPECT_TRUE(status.IsExpressionValidationError()) << status.ToString(); +} + } // namespace gandiva diff --git a/cpp/src/gandiva/tests/projector_test.cc b/cpp/src/gandiva/tests/projector_test.cc index dee184cad759..a556f25f620f 100644 --- a/cpp/src/gandiva/tests/projector_test.cc +++ b/cpp/src/gandiva/tests/projector_test.cc @@ -4047,4 +4047,71 @@ TEST_F(TestProjector, TestRandIntegerWithMinMax) { } } +// Two schemas that differ only in the nullability of a field must not share a cached +// projector, and a batch must match the schema the projector was built with. This +// records the current behavior end to end, through the shared compiled-expression cache. +TEST_F(TestProjector, TestProjectCacheSchemasDifferingInNullability) { + auto nullable_field = field("cache_nullability_f0", int32(), true); + auto required_field = field("cache_nullability_f0", int32(), false); + auto nullable_schema = arrow::schema({nullable_field}); + auto required_schema = arrow::schema({required_field}); + ASSERT_FALSE(nullable_schema->Equals(*required_schema)); + + // The same expression on each schema's own field: f0 + 10. + auto make_expression = [](const FieldPtr& input) { + auto node = TreeExprBuilder::MakeFunction( + "add", {TreeExprBuilder::MakeField(input), TreeExprBuilder::MakeLiteral(10)}, + int32()); + return TreeExprBuilder::MakeExpression(node, field("res", int32())); + }; + auto configuration = TestConfiguration(); + + std::shared_ptr nullable_projector; + ASSERT_OK(Projector::Make(nullable_schema, {make_expression(nullable_field)}, + configuration, &nullable_projector)); + EXPECT_FALSE(nullable_projector->GetBuiltFromCache()); + + // The other schema is a different cache entry, not a hit on the first one. + std::shared_ptr required_projector; + ASSERT_OK(Projector::Make(required_schema, {make_expression(required_field)}, + configuration, &required_projector)); + EXPECT_FALSE(required_projector->GetBuiltFromCache()); + + // Building each one again is a hit on its own entry. + std::shared_ptr nullable_again; + ASSERT_OK(Projector::Make(nullable_schema, {make_expression(nullable_field)}, + configuration, &nullable_again)); + EXPECT_TRUE(nullable_again->GetBuiltFromCache()); + std::shared_ptr required_again; + ASSERT_OK(Projector::Make(required_schema, {make_expression(required_field)}, + configuration, &required_again)); + EXPECT_TRUE(required_again->GetBuiltFromCache()); + + // Each projector evaluates correctly on a batch of its own schema, including a null. + auto nullable_batch = arrow::RecordBatch::Make( + nullable_schema, 3, {MakeArrowArrayInt32({1, 2, 3}, {true, false, true})}); + auto required_batch = + arrow::RecordBatch::Make(required_schema, 3, {MakeArrowArrayInt32({1, 2, 3})}); + auto expected_nullable = MakeArrowArrayInt32({11, 0, 13}, {true, false, true}); + auto expected_required = MakeArrowArrayInt32({11, 12, 13}); + + for (const auto& projector : {nullable_projector, nullable_again}) { + arrow::ArrayVector outputs; + ASSERT_OK(projector->Evaluate(*nullable_batch, pool_, &outputs)); + EXPECT_ARROW_ARRAY_EQUALS(expected_nullable, outputs.at(0)); + } + for (const auto& projector : {required_projector, required_again}) { + arrow::ArrayVector outputs; + ASSERT_OK(projector->Evaluate(*required_batch, pool_, &outputs)); + EXPECT_ARROW_ARRAY_EQUALS(expected_required, outputs.at(0)); + } + + // A batch of the other schema is rejected, in both directions. + arrow::ArrayVector outputs; + auto status = nullable_projector->Evaluate(*required_batch, pool_, &outputs); + EXPECT_TRUE(status.IsInvalid()) << status.ToString(); + status = required_projector->Evaluate(*nullable_batch, pool_, &outputs); + EXPECT_TRUE(status.IsInvalid()) << status.ToString(); +} + } // namespace gandiva