-
Notifications
You must be signed in to change notification settings - Fork 897
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
use like (vs ilike) for service query #18549
Conversation
Checked commit kbrock@26616f8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
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.
This seems fine, though, I was unable to find in my (brief) search for any support in favor or your argument that ILIKE
does not use indexes:
https://www.postgresql.org/docs/9.5/functions-matching.html#FUNCTIONS-LIKE
Mind providing some in the description if there is? Or some other supporting metrics?
@@ -149,7 +149,7 @@ def aggregate_hardware_arel(virtual_column_name, aggregation_sql, options = {}) | |||
def aggregation_where_clause(arel, subtree_services) | |||
arel.grouping( | |||
subtree_services[:id].eq(arel[:id]) | |||
.or(subtree_services[:ancestry].matches(ancestry_ilike)) | |||
.or(subtree_services[:ancestry].matches(ancestry_ilike, nil, true)) |
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.
Also, Keenan clearly knows about this, but for those who might not bother dealing with AREL internals, this is the proper way to flag a case sensitive (LIKE
) query in postgres using AREL:
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.
ILIKE this change. Makes sense to me after @NickLaMuro's information.
@kbrock was there a BZ for this change? Does it need backport labels? |
related to https://bugzilla.redhat.com/show_bug.cgi?id=1686433 please backport to |
The services query uses a potentially ugly subquery for looking up vm totals and stuff.
For ancestry, using
LIKE
is better thanILIKE
- the case sensitivity doesn't provide much when searching numbers and symbols and it prevents us from using text indexes.After this I'll still need to tweak our indexes, but this ilike is preventing us from using anything