-
Notifications
You must be signed in to change notification settings - Fork 1.1k
store/graph: add insensitive matching filters #3294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| | StartsWithInsensitive(attr, _) | ||
| | NotStartsWith(attr, _) | ||
| | NotStartsWithInsensitive(attr, _) | ||
| | EndsWith(attr, _) | ||
| | NotEndsWith(attr, _) => { | ||
| | EndsWithInsensitive(attr, _) | ||
| | NotEndsWith(attr, _) | ||
| | NotEndsWithInsensitive(attr, _) => { | ||
| table.column_for_field(attr)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but it might be a sign that we could use a EntityFilter::attr(&self) -> Option<Attribute> getter method for conciseness.
Maybe we could open a new issue for this.
|
|
||
| let operation = match (strict, negated) { | ||
| (true, true) => " not like ", | ||
| (true, false) => " like ", | ||
| (false, true) => " not ilike ", | ||
| (false, false) => " ilike ", | ||
| }; | ||
| match value { | ||
| Value::String(s) => { | ||
| out.push_identifier(column.name.as_str())?; | ||
| if negated { | ||
| out.push_sql(" not like "); | ||
| } else { | ||
| out.push_sql(" like ") | ||
| }; | ||
| out.push_sql(operation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
This might be bikeshedding, but could we rename the filters from |
|
updated @lutter :) |
lutter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the change!
|
@saihaj can this be merged? |
Yes @otaviopace I don't know if I am allowed to merge or if there are blog post or something we would want for this? |
Closes #3293
Precede #3191