Add has_one scope support to virtual_delegate #17473
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 23, 2018
NickLaMuro
changed the title
kbrock
reviewed
May 29, 2018
| @@ -287,10 +289,22 @@ def virtual_delegate_arel(col, to_ref) | |||
| # | |||
|
|
|||
| def self.select_from_alias(to_ref, col, to_model_col_name, src_model_id) | |||
| to_table = select_from_alias_table(to_ref.klass, src_model_id.relation) | |||
| query = if to_ref.scope | |||
There was a problem hiding this comment.
Very nice to base upon the to_ref association.
This enhances virtual_delegates so that scopes added as part of a
`has_one` relation are respected.
Because of the changes, an implicit `limit = 1` was necessary to support
the `has_one`, and the easiest way to accomplish that while still
keeping `select_from_alias` in a similar form was to allow passing a
block so the arel (prior to being converted to a string) could be
updated.
Also of note, `select_from_alias` also had a few changes to it beyond
that:
- The "query" is now built up from the scope on the model, not a raw
arel_table from the to_ref model.
- Instead of a `project`, we call a `.select` on the "query" (scope
from the relation), which already has a similar arg structure, but
allows us to remove the select_values properly before converting it
to arel (happens right after the `.select`). We work with raw
`Arel` after that.
- The `table_alias` functionality is now done through the `from`
clause, `from(to_table)` allows us to still keep the same alias
functionality we had. Under the covers, this does the same thing,
it is just previously the starting point was an `Arel::Table`, and
now it is a `Arel::SelectManager`. The `Arel::Table` version ends
up returning a `SelectManager` anyway, so what we are doing is
functionally equivalent.
NickLaMuro
force-pushed
the
virtual_delegate_has_one_scope_support
branch
from
eb62cba to
836ea6a
Compare
NickLaMuro
changed the title
NickLaMuro
force-pushed
the
virtual_delegate_has_one_scope_support
branch
from
836ea6a to
fec164a
Compare
|
Checked commits NickLaMuro/manageiq@f33a768~...fec164a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
NickLaMuro
added a commit
to NickLaMuro/manageiq-ui-classic
that referenced
this pull request
Feb 26, 2019
Over time, the `compliances` table will get quiet large, and when paired
with the rest of the tables in this report, you end up getting a query
executed that looks like this:
SELECT "vms"."id" AS t0_r0, ..., "vms"."memory_hot_add_increment" AS t0_r75,
"hosts"."id" AS t1_r0, ..., "hosts"."physical_server_id" AS t1_r36,
"storages"."id" AS t2_r0, ..., "storages"."storage_domain_type" AS t2_r18,
"ext_management_systems"."id" AS t3_r0, ..., "ext_management_systems"."tenant_mapping_enabled" AS t3_r23,
"snapshots"."id" AS t4_r0, ..., "snapshots"."ems_ref" AS t4_r16,
"compliances"."id" AS t5_r0, ..., "compliances"."event_type" AS t5_r6,
"operating_systems"."id" AS t6_r0, ..., "operating_systems"."kernel_version" AS t6_r25,
"hardwares"."id" AS t7_r0, ..., "hardwares"."provision_state" AS t7_r34,
"tags"."id" AS t8_r0, "tags"."name" AS t8_r1
FROM "vms"
LEFT OUTER JOIN "hosts" ON "hosts"."id" = "vms"."host_id"
LEFT OUTER JOIN "storages" ON "storages"."id" = "vms"."storage_id"
LEFT OUTER JOIN "ext_management_systems" ON "ext_management_systems"."id" = "vms"."ems_id"
LEFT OUTER JOIN "snapshots" ON "snapshots"."vm_or_template_id" = "vms"."id"
LEFT OUTER JOIN "compliances" ON "compliances"."resource_id" = "vms"."id" AND "compliances"."resource_type" = $1
LEFT OUTER JOIN "operating_systems" ON "operating_systems"."vm_or_template_id" = "vms"."id"
LEFT OUTER JOIN "hardwares" ON "hardwares"."vm_or_template_id" = "vms"."id"
LEFT OUTER JOIN "taggings" ON "taggings"."taggable_id" = "vms"."id" AND "taggings"."taggable_type" = $2
LEFT OUTER JOIN "tags" ON "tags"."id" = "taggings"."tag_id"
WHERE "vms"."type" IN (...)
AND "vms"."template" = $3
AND (lower(vms.name) like '%win2k%' escape '`')
AND "vms"."id" IN (...)
ORDER BY LOWER("vms"."name") ASC
This ends up creating 243 columns per record, and the number of rows
returned has been observed to be over 80k with just 20 VMs being
targeted in the `"vms"."id" IN (...)` portion of the query.
Since some fixes have been made to avoid the N+1 that results from doing
this:
- ManageIQ/manageiq#17473
- ManageIQ/manageiq#17474
- ManageIQ/manageiq#17475
It is much better to do use those facilities. Even with the N+1, it is
much better making extra round trips than the gigs of data that would be
returned as a result.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This enhances virtual_delegates so that scopes added as part of a
has_onerelation are respected.Because of the changes, an implicit
limit = 1was necessary to support thehas_one, and the easiest way to accomplish that while still keepingselect_from_aliasin a similar form was to allow passing a block so the arel (prior to being converted to a string) could be updated.Also of note,
select_from_aliasalso had a few changes to it beyond that:arel_tablefrom theto_refmodel.project, we call a.selecton the "query" (scope from the relation), which already has a similar arg structure, but allows us to remove theselect_valuesproperly before converting it to arel (happens right after the.select). We work with rawArelafter that.table_aliasfunctionality is now done through thefromclause,from(to_table)allows us to still keep the same alias functionality we had. Under the covers, this does the same thing, it is just previously the starting point was anArel::Table, and now it is aArel::SelectManager. TheArel::Tableversion ends up returning aSelectManageranyway, so what we are doing is functionally equivalent.Links
queryfrom arelationbit that I used previously, and again in this PRTODO
Should probably add some tests around this. That said, this is a for a semi POC for an alternative to #17469, so the POC should probably be validated against before commiting to the changes here.