Repository navigation
GH-50988: [C++][Python] Substrait: add mappings for starts_with, ends_with and match_substring - #50989
Conversation
…, 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.
|
|
github-actions
Bot
added
Component: C++
Component: Python
Component: Documentation
labels
|
CI failures:
=> I believe the CI failures are not because of changes introduced in this PR. |
* 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
|
@kou @felipecrv could you take a look when you have a chance? This adds Substrait mappings for I trimmed the PR down a bit in 7ec5d6d. |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
+1
@zanmato1984 Do you want to review this?
| substrait_call.SetValueArg(0, call.arguments[0]); | ||
| substrait_call.SetValueArg(1, compute::literal(match_options->pattern)); |
There was a problem hiding this comment.
Should we check call.arguments[0] type and match_options->pattern type because Substrait accepts only string but the compute module accepts non-string like binary?
There was a problem hiding this comment.
Good catch. I think we should check the bound input type before using the standard Substrait string mapping. These Arrow kernels also accept binary and large binary/string inputs, but functions_string.yaml has no matching signatures for those types, so the current encoder can produce an invalid standard Substrait call. With the current type mappings, I think we should only accept plain string input here, return NotImplemented for unsupported types, and add a binary regression test. We should also ensure the pattern is valid for a Substrait string literal.
There was a problem hiding this comment.
Good point, done. The encoder now requires utf8 input and a valid UTF-8 pattern, and returns NotImplemented otherwise. large_utf8 is rejected too, because it's serialized as a user-defined type that wouldn't match Substrait's string/varchar signatures.
github-actions
Bot
added
awaiting review
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.
zanmato1984
left a comment
zanmato1984
left a comment
There was a problem hiding this comment.
+1
Thanks for working on this.
3 participants
Rationale for this change
starts_with,ends_withandmatch_substringhave no Substrait mapping, so any expression using them fails to serialize:Comparisons,
isinand arithmetic serialize fine, so this is a per-function gap. It matters for engines that ingest a PyArrow filter through Substrait, where an unmappable function becomes a hard failure rather than a fallback.See #50988.
What changes are included in this PR?
Map the three kernels onto
starts_with,ends_withandcontainsfrom Substrait'sfunctions_string.yaml, in both directions.The signatures do not line up. Substrait passes the pattern as a second argument, while the Arrow kernels are unary and carry it in
MatchSubstringOptions. So:MatchSubstringOptions::patternout into a literal argumentNotImplementedotherwiseSubstrait's
case_sensitivityoption maps ontoignore_case.CASE_INSENSITIVE_ASCIIhas no Arrow equivalent and returnsNotImplemented.Both caveats are documented in
docs/source/cpp/acero/substrait.rst.Are these changes tested?
Yes.
Substrait.StringMatchExpressionSerializationround-trips all three functions with and withoutignore_case.Substrait.StringMatchExpressionDeserializationdeserializes hand-written Substrait JSON to cover the default (no option) case and the two rejection paths.test_serializing_string_match_expressionsround-trips each function throughpyarrow.substrait.Are there any user-facing changes?
Yes.
pc.starts_with,pc.ends_withandpc.match_substringcan now be serialized to Substrait and consumed back, and Acero can consume plans using Substrait'sstarts_with,ends_withandcontains. No existing behaviour changes: these previously raised.