diff --git a/cpp/src/gandiva/cache_test.cc b/cpp/src/gandiva/cache_test.cc index d371db59dfc..6e64ca518a1 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,304 @@ 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, 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( + "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/expression_cache_key.h b/cpp/src/gandiva/expression_cache_key.h index e7522042a7d..052522d24ca 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; } } diff --git a/cpp/src/gandiva/tests/boolean_expr_test.cc b/cpp/src/gandiva/tests/boolean_expr_test.cc index a54f7622ba2..0bb74145e38 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/filter_test.cc b/cpp/src/gandiva/tests/filter_test.cc index 749000aa0cf..68e066e390a 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/if_expr_test.cc b/cpp/src/gandiva/tests/if_expr_test.cc index 54b6d43b4df..542968556bd 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 1ed4c77a074..f8c4cd6428f 100644 --- a/cpp/src/gandiva/tests/projector_build_validation_test.cc +++ b/cpp/src/gandiva/tests/projector_build_validation_test.cc @@ -284,4 +284,67 @@ 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(); +} + +// 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 dee184cad75..a556f25f620 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