Skip to content

Conversation

@lidavidm
Copy link
Member

Implements a simple SQL LIKE pattern match kernel by translating it to a regex (or substring) match as appropriate.

@github-actions
Copy link

Copy link
Member

@ianmcook ianmcook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for doing this @lidavidm!

@ianmcook
Copy link
Member

P.S. case insensitivity can be achieved by using one of the _upper or _lower kernels, which I think is good enough

@lidavidm
Copy link
Member Author

We could easily add a flag to toggle insensitivity in RE2 as well. (Would be better than allocating a new string and potentially dealing with Unicode.)

@ianmcook
Copy link
Member

But can PlainSubstringMatcher do case-insensitive matching?

@lidavidm
Copy link
Member Author

No, but in that case we'd just not dispatch to that path.

@ianmcook
Copy link
Member

What to do about the options class might be awkward. You could subclass MatchSubstringOptions, but I think ultimately we might incorporate an option for case insensitivity into MatchSubstringOptions.

@lidavidm
Copy link
Member Author

We could add it to MatchSubstringOptions, if we plan to support for all kernels, and leave it unimplemented for now in the non-regex case (it's pretty easy to support in any regex case).

@ianmcook
Copy link
Member

Sounds good. The case-(in)sensitive option should be optional and should default to case-sensitive. We should error in cases where case-insensitive match is specified in the options but is ignored by a kernel or by any path in a kernel. This sounds to me like a separate PR. Should I open a Jira for that?

@lidavidm
Copy link
Member Author

I filed ARROW-12835. I think this PR should be otherwise good-to-go then, unless we need to also add R bindings.

@ianmcook
Copy link
Member

I think it's good to go. Nothing is needed in the R bindings.

@lidavidm lidavidm closed this in d07f30a May 20, 2021
@lidavidm
Copy link
Member Author

Merged, thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants