From e75aea5a6dceade8cb70e0e620b12c86178b3a48 Mon Sep 17 00:00:00 2001 From: Jonas Dedden Date: Tue, 25 Aug 2026 16:53:38 +0200 Subject: [PATCH 1/3] GH-50988: [C++][Python] Substrait: add mappings for starts_with, ends_with and match_substring The Arrow kernels starts_with, ends_with and match_substring had no Substrait mapping, so serialize_expressions raised ArrowNotImplementedError for any expression using them. Map them onto the starts_with, ends_with and contains functions in Substrait's functions_string.yaml. The signatures differ: Substrait takes the pattern as a second argument while the Arrow kernels are unary and carry it in MatchSubstringOptions, so the decoder requires that argument to be a non-null string literal. Substrait's case_sensitivity option maps onto ignore_case; CASE_INSENSITIVE_ASCII has no Arrow equivalent and returns NotImplemented. --- .../arrow/engine/substrait/extension_set.cc | 97 +++++++++++++++ cpp/src/arrow/engine/substrait/serde_test.cc | 112 ++++++++++++++++++ docs/source/cpp/acero/substrait.rst | 5 + python/pyarrow/tests/test_substrait.py | 23 ++++ 4 files changed, 237 insertions(+) diff --git a/cpp/src/arrow/engine/substrait/extension_set.cc b/cpp/src/arrow/engine/substrait/extension_set.cc index 4fb9b19645b1..9e9492d46227 100644 --- a/cpp/src/arrow/engine/substrait/extension_set.cc +++ b/cpp/src/arrow/engine/substrait/extension_set.cc @@ -26,8 +26,10 @@ #include "arrow/compute/api_scalar.h" #include "arrow/engine/substrait/options.h" +#include "arrow/scalar.h" #include "arrow/type.h" #include "arrow/type_fwd.h" +#include "arrow/type_traits.h" #include "arrow/util/checked_cast.h" #include "arrow/util/hash_util.h" #include "arrow/util/hashing.h" @@ -36,6 +38,7 @@ namespace arrow { +using internal::checked_cast; using internal::checked_pointer_cast; namespace engine { namespace { @@ -757,6 +760,19 @@ static std::vector kRoundModes = { "TIE_UP", "TIE_TOWARDS_ZERO", "TIE_AWAY_FROM_ZERO", "TIE_TO_EVEN", "TIE_TO_ODD"}; static EnumParser kRoundModeParser(kRoundModes); +// The case_sensitivity option used by the Substrait string functions +// (starts_with / ends_with / contains). Arrow's MatchSubstringOptions only +// distinguishes case sensitive from case insensitive, so CASE_INSENSITIVE_ASCII +// is parsed but not implemented. +enum class CaseSensitivity { + kCaseSensitive = 0, + kCaseInsensitive, + kCaseInsensitiveAscii +}; +static std::vector kCaseSensitivityOptions = { + "CASE_SENSITIVE", "CASE_INSENSITIVE", "CASE_INSENSITIVE_ASCII"}; +static EnumParser kCaseSensitivityParser(kCaseSensitivityOptions); + template Result ParseOptionOrElse(const SubstraitCall& call, std::string_view option_name, const EnumParser& parser, @@ -961,6 +977,71 @@ ExtensionIdRegistry::SubstraitCallToArrow DecodeConcatMapping() { }; } +// Substrait's starts_with / ends_with / contains take the pattern as a second +// value argument. The matching Arrow kernels (starts_with / ends_with / +// match_substring) are unary and carry the pattern in MatchSubstringOptions, so +// the second argument must be a string literal. +ExtensionIdRegistry::SubstraitCallToArrow DecodeMatchSubstringMapping( + const std::string& function_name) { + return [function_name](const SubstraitCall& call) -> Result { + if (call.size() != 2) { + return Status::NotImplemented("Acero does not have a kernel for ", function_name, + " that receives ", call.size(), " arguments"); + } + ARROW_ASSIGN_OR_RAISE( + CaseSensitivity case_sensitivity, + ParseOptionOrElse( + call, "case_sensitivity", kCaseSensitivityParser, + {CaseSensitivity::kCaseSensitive, CaseSensitivity::kCaseInsensitive}, + CaseSensitivity::kCaseSensitive)); + ARROW_ASSIGN_OR_RAISE(compute::Expression input, call.GetValueArg(0)); + ARROW_ASSIGN_OR_RAISE(compute::Expression substring, call.GetValueArg(1)); + const Datum* pattern = substring.literal(); + if (pattern == nullptr || !pattern->is_scalar()) { + return Status::NotImplemented( + "The Arrow ", function_name, + " kernel requires the substring argument to be a literal"); + } + const std::shared_ptr& pattern_scalar = pattern->scalar(); + if (!pattern_scalar->is_valid || (!is_base_binary_like(pattern_scalar->type->id()) && + !is_binary_view_like(pattern_scalar->type->id()))) { + return Status::NotImplemented( + "The Arrow ", function_name, + " kernel requires the substring argument to be a non-null string literal"); + } + auto options = std::make_shared( + std::string(checked_cast(*pattern_scalar).view()), + /*ignore_case=*/case_sensitivity == CaseSensitivity::kCaseInsensitive); + return compute::call(function_name, {std::move(input)}, std::move(options)); + }; +} + +ExtensionIdRegistry::ArrowToSubstraitCall EncodeMatchSubstring(Id substrait_fn_id) { + return + [substrait_fn_id](const compute::Expression::Call& call) -> Result { + if (call.arguments.size() != 1) { + return Status::Invalid("Expected the call to ", call.function_name, + " to have exactly one argument but it had ", + call.arguments.size()); + } + if (call.options == nullptr) { + return Status::Invalid("The call to ", call.function_name, + " is missing its MatchSubstringOptions"); + } + auto match_options = + checked_pointer_cast(call.options); + // nullable=true errs on the side of caution + SubstraitCall substrait_call(substrait_fn_id, call.type.GetSharedPtr(), + /*nullable=*/true); + substrait_call.SetValueArg(0, call.arguments[0]); + substrait_call.SetValueArg(1, compute::literal(match_options->pattern)); + substrait_call.SetOption( + "case_sensitivity", + {match_options->ignore_case ? "CASE_INSENSITIVE" : "CASE_SENSITIVE"}); + return substrait_call; + }; +} + ExtensionIdRegistry::SubstraitAggregateToArrow DecodeBasicAggregate( const std::string& arrow_function_name) { return [arrow_function_name](const SubstraitCall& call) -> Result { @@ -1147,6 +1228,13 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl { DecodeTemporalExtractionMapping())); DCHECK_OK(AddSubstraitCallToArrow({kSubstraitStringFunctionsUri, "concat"}, DecodeConcatMapping())); + for (const auto& fn_pair : std::vector>{ + {"starts_with", "starts_with"}, + {"ends_with", "ends_with"}, + {"contains", "match_substring"}}) { + DCHECK_OK(AddSubstraitCallToArrow({kSubstraitStringFunctionsUri, fn_pair.first}, + DecodeMatchSubstringMapping(fn_pair.second))); + } DCHECK_OK( AddSubstraitCallToArrow({kSubstraitComparisonFunctionsUri, "is_null"}, DecodeOptionlessBasicMapping("is_null", /*max_args=*/1))); @@ -1239,6 +1327,15 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl { DCHECK_OK(AddArrowToSubstraitCall( "is_null", EncodeIsNull({kSubstraitComparisonFunctionsUri, "is_null"}))); + + for (const auto& fn_pair : std::vector>{ + {"starts_with", "starts_with"}, + {"ends_with", "ends_with"}, + {"match_substring", "contains"}}) { + DCHECK_OK(AddArrowToSubstraitCall( + fn_pair.first, + EncodeMatchSubstring({kSubstraitStringFunctionsUri, fn_pair.second}))); + } } }; diff --git a/cpp/src/arrow/engine/substrait/serde_test.cc b/cpp/src/arrow/engine/substrait/serde_test.cc index 4de5480ef3cd..8a67934bf238 100644 --- a/cpp/src/arrow/engine/substrait/serde_test.cc +++ b/cpp/src/arrow/engine/substrait/serde_test.cc @@ -6377,5 +6377,117 @@ TEST(Substrait, ExtendedExpressionInvalidPlans) { Raises(StatusCode::Invalid, testing::HasSubstr("Ambiguous plan"))); } +TEST(Substrait, StringMatchExpressionSerialization) { + std::shared_ptr test_schema = schema({field("cat", utf8())}); + for (const auto& fn_name : {"starts_with", "ends_with", "match_substring"}) { + for (bool ignore_case : {false, true}) { + CheckExpressionRoundTrip( + *test_schema, compute::call(fn_name, {compute::field_ref(0)}, + compute::MatchSubstringOptions("al", ignore_case))); + } + } + // The pattern survives as a nested expression too + CheckExpressionRoundTrip( + *test_schema, + compute::call("invert", {compute::call("starts_with", {compute::field_ref(0)}, + compute::MatchSubstringOptions("al"))})); +} + +namespace { + +// An ExtendedExpression holding a single starts_with call over a utf8 column named +// "cat". `arguments` and `options` are spliced into the scalar function so that +// individual tests can vary them. +std::string StartsWithExpressionJSON(std::string_view arguments, + std::string_view options) { + return R"({ + "extensionUris":[ + { + "extensionUriAnchor":1, + "uri":")" + + std::string(kSubstraitStringFunctionsUri) + R"(" + } + ], + "extensions":[ + { + "extensionFunction":{ + "extensionUriReference":1, + "functionAnchor":1, + "name":"starts_with" + } + } + ], + "referredExpr":[ + { + "expression":{ + "scalarFunction":{ + "functionReference":1, + "outputType":{"bool":{"nullability":"NULLABILITY_NULLABLE"}}, + "arguments":)" + + std::string(arguments) + R"(, + "options":)" + + std::string(options) + R"( + } + }, + "outputNames":["out"] + } + ], + "baseSchema":{ + "names":["cat"], + "struct":{ + "types":[{"string":{"nullability":"NULLABILITY_NULLABLE"}}] + } + }, + "version":{"majorNumber":9999} + })"; +} + +constexpr std::string_view kCatAndLiteralArgs = R"([ + {"value":{"selection":{"directReference":{"structField":{"field":0}}, + "rootReference":{}}}}, + {"value":{"literal":{"string":"al"}}} +])"; + +} // namespace + +TEST(Substrait, StringMatchExpressionDeserialization) { + auto deserialize = [](const std::string& json) -> Result { + ARROW_ASSIGN_OR_RAISE(std::shared_ptr buf, + internal::SubstraitFromJSON("ExtendedExpression", json)); + return DeserializeExpressions(*buf); + }; + + std::shared_ptr test_schema = schema({field("cat", utf8())}); + + // No case_sensitivity option means case sensitive + ASSERT_OK_AND_ASSIGN(BoundExpressions no_option, + deserialize(StartsWithExpressionJSON(kCatAndLiteralArgs, "[]"))); + ASSERT_OK_AND_ASSIGN(compute::Expression expected, + compute::call("starts_with", {compute::field_ref(0)}, + compute::MatchSubstringOptions("al")) + .Bind(*test_schema)); + ASSERT_EQ(1, no_option.named_expressions.size()); + ASSERT_EQ(expected, no_option.named_expressions[0].expression); + + // CASE_INSENSITIVE_ASCII has no Arrow equivalent + ASSERT_THAT( + deserialize(StartsWithExpressionJSON( + kCatAndLiteralArgs, + R"([{"name":"case_sensitivity","preference":["CASE_INSENSITIVE_ASCII"]}])")), + Raises(StatusCode::NotImplemented, + testing::HasSubstr("the only supported options are"))); + + // The pattern has to be a literal because the Arrow kernel takes it as an option + constexpr std::string_view kTwoFieldArgs = R"([ + {"value":{"selection":{"directReference":{"structField":{"field":0}}, + "rootReference":{}}}}, + {"value":{"selection":{"directReference":{"structField":{"field":0}}, + "rootReference":{}}}} + ])"; + ASSERT_THAT(deserialize(StartsWithExpressionJSON(kTwoFieldArgs, "[]")), + Raises(StatusCode::NotImplemented, + testing::HasSubstr("substring argument to be a literal"))); +} + } // namespace engine } // namespace arrow diff --git a/docs/source/cpp/acero/substrait.rst b/docs/source/cpp/acero/substrait.rst index a5532733627c..3322a41a362a 100644 --- a/docs/source/cpp/acero/substrait.rst +++ b/docs/source/cpp/acero/substrait.rst @@ -227,6 +227,11 @@ Functions * Acero does not support the SATURATE option for overflow * Acero does not support kernels that take more than two arguments for the functions ``and``, ``or``, ``xor`` + * The functions ``starts_with``, ``ends_with``, and ``contains`` map onto Acero + kernels that take the substring as a function option. The second argument + must therefore be a non-null string literal. The ``case_sensitivity`` option + supports ``CASE_SENSITIVE`` and ``CASE_INSENSITIVE`` but not + ``CASE_INSENSITIVE_ASCII`` * Substrait has not yet clearly identified the form that URIs should take for standard functions. Acero will look for the URIs to the ``main`` GitHub branch. diff --git a/python/pyarrow/tests/test_substrait.py b/python/pyarrow/tests/test_substrait.py index fcd1c8d48c5f..e5b0f5eb36ac 100644 --- a/python/pyarrow/tests/test_substrait.py +++ b/python/pyarrow/tests/test_substrait.py @@ -946,6 +946,29 @@ def test_serializing_expressions(expr): assert "test_expr" in returned.expressions +@pytest.mark.parametrize("make_expr", [ + lambda f: pc.starts_with(f, "pre"), + lambda f: pc.ends_with(f, "post"), + lambda f: pc.match_substring(f, "mid"), + lambda f: pc.starts_with(f, "pre", ignore_case=True), + lambda f: pc.ends_with(f, "post", ignore_case=True), + lambda f: pc.match_substring(f, "mid", ignore_case=True), + lambda f: ~pc.starts_with(f, "pre"), +]) +def test_serializing_string_match_expressions(make_expr): + # GH-50988: starts_with / ends_with / match_substring map onto the Substrait + # starts_with / ends_with / contains functions + schema = pa.schema([pa.field("s", pa.string())]) + + buf = pa.substrait.serialize_expressions( + [make_expr(pc.field("s"))], ["test_expr"], schema) + returned = pa.substrait.deserialize_expressions(buf) + assert schema == returned.schema + assert len(returned.expressions) == 1 + # Substrait refers to fields by index, so compare against the normalized form + assert str(returned.expressions["test_expr"]) == str(make_expr(pc.field(0))) + + def test_arrow_specific_types(): fields = { "time_seconds": (pa.time32("s"), 0), From 7ec5d6dd77eaadfe3b2185b7afc5ec3b6953db22 Mon Sep 17 00:00:00 2001 From: Jonas Dedden Date: Sun, 27 Sep 2026 14:28:01 +0300 Subject: [PATCH 2/3] Trim tests and simplify string literal check * Deserialize a bare Expression against an ExtensionSet instead of hand-writing a full ExtendedExpression * Drop the nested invert() round trip, which only exercises generic call serialization * Substrait string literals always decode to utf8, so check for that directly * Drop the unreachable arity check in the encoder * Reduce the Python test to one case per function --- .../arrow/engine/substrait/extension_set.cc | 19 +-- cpp/src/arrow/engine/substrait/serde_test.cc | 119 +++++------------- python/pyarrow/tests/test_substrait.py | 24 ++-- 3 files changed, 40 insertions(+), 122 deletions(-) diff --git a/cpp/src/arrow/engine/substrait/extension_set.cc b/cpp/src/arrow/engine/substrait/extension_set.cc index 9e9492d46227..02b571735154 100644 --- a/cpp/src/arrow/engine/substrait/extension_set.cc +++ b/cpp/src/arrow/engine/substrait/extension_set.cc @@ -29,7 +29,6 @@ #include "arrow/scalar.h" #include "arrow/type.h" #include "arrow/type_fwd.h" -#include "arrow/type_traits.h" #include "arrow/util/checked_cast.h" #include "arrow/util/hash_util.h" #include "arrow/util/hashing.h" @@ -38,7 +37,6 @@ namespace arrow { -using internal::checked_cast; using internal::checked_pointer_cast; namespace engine { namespace { @@ -997,20 +995,14 @@ ExtensionIdRegistry::SubstraitCallToArrow DecodeMatchSubstringMapping( ARROW_ASSIGN_OR_RAISE(compute::Expression input, call.GetValueArg(0)); ARROW_ASSIGN_OR_RAISE(compute::Expression substring, call.GetValueArg(1)); const Datum* pattern = substring.literal(); - if (pattern == nullptr || !pattern->is_scalar()) { - return Status::NotImplemented( - "The Arrow ", function_name, - " kernel requires the substring argument to be a literal"); - } - const std::shared_ptr& pattern_scalar = pattern->scalar(); - if (!pattern_scalar->is_valid || (!is_base_binary_like(pattern_scalar->type->id()) && - !is_binary_view_like(pattern_scalar->type->id()))) { + if (pattern == nullptr || !pattern->is_scalar() || + pattern->type()->id() != Type::STRING || !pattern->scalar()->is_valid) { return Status::NotImplemented( "The Arrow ", function_name, " kernel requires the substring argument to be a non-null string literal"); } auto options = std::make_shared( - std::string(checked_cast(*pattern_scalar).view()), + std::string(pattern->scalar_as().view()), /*ignore_case=*/case_sensitivity == CaseSensitivity::kCaseInsensitive); return compute::call(function_name, {std::move(input)}, std::move(options)); }; @@ -1019,11 +1011,6 @@ ExtensionIdRegistry::SubstraitCallToArrow DecodeMatchSubstringMapping( ExtensionIdRegistry::ArrowToSubstraitCall EncodeMatchSubstring(Id substrait_fn_id) { return [substrait_fn_id](const compute::Expression::Call& call) -> Result { - if (call.arguments.size() != 1) { - return Status::Invalid("Expected the call to ", call.function_name, - " to have exactly one argument but it had ", - call.arguments.size()); - } if (call.options == nullptr) { return Status::Invalid("The call to ", call.function_name, " is missing its MatchSubstringOptions"); diff --git a/cpp/src/arrow/engine/substrait/serde_test.cc b/cpp/src/arrow/engine/substrait/serde_test.cc index 8a67934bf238..8c9ba9a53044 100644 --- a/cpp/src/arrow/engine/substrait/serde_test.cc +++ b/cpp/src/arrow/engine/substrait/serde_test.cc @@ -6386,107 +6386,48 @@ TEST(Substrait, StringMatchExpressionSerialization) { compute::MatchSubstringOptions("al", ignore_case))); } } - // The pattern survives as a nested expression too - CheckExpressionRoundTrip( - *test_schema, - compute::call("invert", {compute::call("starts_with", {compute::field_ref(0)}, - compute::MatchSubstringOptions("al"))})); } -namespace { - -// An ExtendedExpression holding a single starts_with call over a utf8 column named -// "cat". `arguments` and `options` are spliced into the scalar function so that -// individual tests can vary them. -std::string StartsWithExpressionJSON(std::string_view arguments, - std::string_view options) { - return R"({ - "extensionUris":[ - { - "extensionUriAnchor":1, - "uri":")" + - std::string(kSubstraitStringFunctionsUri) + R"(" - } - ], - "extensions":[ - { - "extensionFunction":{ - "extensionUriReference":1, - "functionAnchor":1, - "name":"starts_with" - } - } - ], - "referredExpr":[ - { - "expression":{ - "scalarFunction":{ - "functionReference":1, - "outputType":{"bool":{"nullability":"NULLABILITY_NULLABLE"}}, - "arguments":)" + - std::string(arguments) + R"(, - "options":)" + - std::string(options) + R"( - } - }, - "outputNames":["out"] - } - ], - "baseSchema":{ - "names":["cat"], - "struct":{ - "types":[{"string":{"nullability":"NULLABILITY_NULLABLE"}}] - } - }, - "version":{"majorNumber":9999} - })"; -} - -constexpr std::string_view kCatAndLiteralArgs = R"([ - {"value":{"selection":{"directReference":{"structField":{"field":0}}, - "rootReference":{}}}}, - {"value":{"literal":{"string":"al"}}} -])"; - -} // namespace - TEST(Substrait, StringMatchExpressionDeserialization) { - auto deserialize = [](const std::string& json) -> Result { + ExtensionSet ext_set; + ASSERT_OK_AND_ASSIGN( + uint32_t anchor, + ext_set.EncodeFunction({kSubstraitStringFunctionsUri, "starts_with"})); + // A starts_with call on field 0 with the given pattern argument and options + auto deserialize = [&](std::string_view pattern_arg, + std::string_view options) -> Result { + std::string json = R"({"scalarFunction":{"functionReference":)" + + std::to_string(anchor) + + R"(,"outputType":{"bool":{}},"arguments":[ + {"value":{"selection":{"directReference":{"structField":{"field":0}}, + "rootReference":{}}}}, + {"value":)" + std::string(pattern_arg) + + R"(}],"options":)" + std::string(options) + "}}"; ARROW_ASSIGN_OR_RAISE(std::shared_ptr buf, - internal::SubstraitFromJSON("ExtendedExpression", json)); - return DeserializeExpressions(*buf); + internal::SubstraitFromJSON("Expression", json)); + return DeserializeExpression(*buf, ext_set); }; - - std::shared_ptr test_schema = schema({field("cat", utf8())}); + constexpr std::string_view kLiteral = R"({"literal":{"string":"al"}})"; // No case_sensitivity option means case sensitive - ASSERT_OK_AND_ASSIGN(BoundExpressions no_option, - deserialize(StartsWithExpressionJSON(kCatAndLiteralArgs, "[]"))); - ASSERT_OK_AND_ASSIGN(compute::Expression expected, - compute::call("starts_with", {compute::field_ref(0)}, - compute::MatchSubstringOptions("al")) - .Bind(*test_schema)); - ASSERT_EQ(1, no_option.named_expressions.size()); - ASSERT_EQ(expected, no_option.named_expressions[0].expression); + ASSERT_OK_AND_ASSIGN(compute::Expression no_option, deserialize(kLiteral, "[]")); + ASSERT_EQ(compute::call("starts_with", {compute::field_ref(0)}, + compute::MatchSubstringOptions("al")), + no_option); // CASE_INSENSITIVE_ASCII has no Arrow equivalent ASSERT_THAT( - deserialize(StartsWithExpressionJSON( - kCatAndLiteralArgs, - R"([{"name":"case_sensitivity","preference":["CASE_INSENSITIVE_ASCII"]}])")), - Raises(StatusCode::NotImplemented, - testing::HasSubstr("the only supported options are"))); + deserialize( + kLiteral, + R"([{"name":"case_sensitivity","preference":["CASE_INSENSITIVE_ASCII"]}])"), + Raises(StatusCode::NotImplemented, HasSubstr("the only supported options are"))); // The pattern has to be a literal because the Arrow kernel takes it as an option - constexpr std::string_view kTwoFieldArgs = R"([ - {"value":{"selection":{"directReference":{"structField":{"field":0}}, - "rootReference":{}}}}, - {"value":{"selection":{"directReference":{"structField":{"field":0}}, - "rootReference":{}}}} - ])"; - ASSERT_THAT(deserialize(StartsWithExpressionJSON(kTwoFieldArgs, "[]")), - Raises(StatusCode::NotImplemented, - testing::HasSubstr("substring argument to be a literal"))); + ASSERT_THAT( + deserialize(R"({"selection":{"directReference":{"structField":{"field":0}}, + "rootReference":{}}})", + "[]"), + Raises(StatusCode::NotImplemented, HasSubstr("substring argument to be a"))); } } // namespace engine diff --git a/python/pyarrow/tests/test_substrait.py b/python/pyarrow/tests/test_substrait.py index e5b0f5eb36ac..430bbeb65248 100644 --- a/python/pyarrow/tests/test_substrait.py +++ b/python/pyarrow/tests/test_substrait.py @@ -946,27 +946,17 @@ def test_serializing_expressions(expr): assert "test_expr" in returned.expressions -@pytest.mark.parametrize("make_expr", [ - lambda f: pc.starts_with(f, "pre"), - lambda f: pc.ends_with(f, "post"), - lambda f: pc.match_substring(f, "mid"), - lambda f: pc.starts_with(f, "pre", ignore_case=True), - lambda f: pc.ends_with(f, "post", ignore_case=True), - lambda f: pc.match_substring(f, "mid", ignore_case=True), - lambda f: ~pc.starts_with(f, "pre"), -]) -def test_serializing_string_match_expressions(make_expr): - # GH-50988: starts_with / ends_with / match_substring map onto the Substrait - # starts_with / ends_with / contains functions +@pytest.mark.parametrize("func", [pc.starts_with, pc.ends_with, pc.match_substring]) +def test_serializing_string_match_expressions(func): + # GH-50988: these map onto Substrait's starts_with / ends_with / contains schema = pa.schema([pa.field("s", pa.string())]) + expr = func(pc.field("s"), "al", ignore_case=True) - buf = pa.substrait.serialize_expressions( - [make_expr(pc.field("s"))], ["test_expr"], schema) + buf = pa.substrait.serialize_expressions([expr], ["test_expr"], schema) returned = pa.substrait.deserialize_expressions(buf) - assert schema == returned.schema - assert len(returned.expressions) == 1 # Substrait refers to fields by index, so compare against the normalized form - assert str(returned.expressions["test_expr"]) == str(make_expr(pc.field(0))) + assert str(returned.expressions["test_expr"]) == str( + func(pc.field(0), "al", ignore_case=True)) def test_arrow_specific_types(): From 3ca07b7c98f04d7d7c2f2e3b562c1b83c4327411 Mon Sep 17 00:00:00 2001 From: Jonas Dedden Date: Mon, 28 Sep 2026 08:30:41 +0300 Subject: [PATCH 3/3] Only encode string match calls on string input with a UTF-8 pattern Substrait's starts_with / ends_with / contains only accept strings while the Arrow kernels also accept binary-like input, and MatchSubstringOptions may hold a pattern that is not valid UTF-8. Return NotImplemented for those instead of producing an invalid Substrait call. --- .../arrow/engine/substrait/extension_set.cc | 15 +++++++++++++++ cpp/src/arrow/engine/substrait/serde_test.cc | 19 +++++++++++++++++++ docs/source/cpp/acero/substrait.rst | 3 ++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/engine/substrait/extension_set.cc b/cpp/src/arrow/engine/substrait/extension_set.cc index 02b571735154..78188dd8d72e 100644 --- a/cpp/src/arrow/engine/substrait/extension_set.cc +++ b/cpp/src/arrow/engine/substrait/extension_set.cc @@ -34,6 +34,7 @@ #include "arrow/util/hashing.h" #include "arrow/util/logging_internal.h" #include "arrow/util/string.h" +#include "arrow/util/utf8.h" namespace arrow { @@ -1015,8 +1016,22 @@ ExtensionIdRegistry::ArrowToSubstraitCall EncodeMatchSubstring(Id substrait_fn_i return Status::Invalid("The call to ", call.function_name, " is missing its MatchSubstringOptions"); } + // Substrait's starts_with / ends_with / contains only accept strings while + // the Arrow kernels also accept binary-like input and non-UTF-8 patterns + const DataType* input_type = call.arguments[0].type(); + if (input_type == nullptr || input_type->id() != Type::STRING) { + return Status::NotImplemented( + "Substrait only supports ", substrait_fn_id.name, + " on string input but the input to ", call.function_name, " is ", + input_type == nullptr ? "unbound" : input_type->ToString()); + } auto match_options = checked_pointer_cast(call.options); + util::InitializeUTF8(); + if (!util::ValidateUTF8(match_options->pattern)) { + return Status::NotImplemented("Substrait only supports ", substrait_fn_id.name, + " with a valid UTF-8 pattern"); + } // nullable=true errs on the side of caution SubstraitCall substrait_call(substrait_fn_id, call.type.GetSharedPtr(), /*nullable=*/true); diff --git a/cpp/src/arrow/engine/substrait/serde_test.cc b/cpp/src/arrow/engine/substrait/serde_test.cc index 8c9ba9a53044..311876379830 100644 --- a/cpp/src/arrow/engine/substrait/serde_test.cc +++ b/cpp/src/arrow/engine/substrait/serde_test.cc @@ -6386,6 +6386,25 @@ TEST(Substrait, StringMatchExpressionSerialization) { compute::MatchSubstringOptions("al", ignore_case))); } } + + // Substrait only accepts string input and UTF-8 patterns + auto serialize = [](const std::shared_ptr& type, + std::string pattern) -> Result> { + Schema schema({field("s", type)}); + ExtensionSet ext_set; + ARROW_ASSIGN_OR_RAISE( + compute::Expression bound, + compute::call("starts_with", {compute::field_ref(0)}, + compute::MatchSubstringOptions(std::move(pattern))) + .Bind(schema)); + return SerializeExpression(bound, &ext_set); + }; + ASSERT_THAT(serialize(binary(), "al"), + Raises(StatusCode::NotImplemented, HasSubstr("on string input"))); + ASSERT_THAT(serialize(large_utf8(), "al"), + Raises(StatusCode::NotImplemented, HasSubstr("on string input"))); + ASSERT_THAT(serialize(utf8(), "\xff"), + Raises(StatusCode::NotImplemented, HasSubstr("valid UTF-8 pattern"))); } TEST(Substrait, StringMatchExpressionDeserialization) { diff --git a/docs/source/cpp/acero/substrait.rst b/docs/source/cpp/acero/substrait.rst index 3322a41a362a..168cd9b5af37 100644 --- a/docs/source/cpp/acero/substrait.rst +++ b/docs/source/cpp/acero/substrait.rst @@ -231,7 +231,8 @@ Functions kernels that take the substring as a function option. The second argument must therefore be a non-null string literal. The ``case_sensitivity`` option supports ``CASE_SENSITIVE`` and ``CASE_INSENSITIVE`` but not - ``CASE_INSENSITIVE_ASCII`` + ``CASE_INSENSITIVE_ASCII``. Only calls on ``string`` input with a valid + UTF-8 pattern can be serialized to Substrait * Substrait has not yet clearly identified the form that URIs should take for standard functions. Acero will look for the URIs to the ``main`` GitHub branch.