These are chat archives for ManageIQ/manageiq/performance

8th
Apr 2016
Keenan Brock
@kbrock
Apr 08 2016 04:16
@matthewd for the record, I tried about 8 ways to pass in the scope to preload. And your "JustWork" solution nailed it on the first time. thanks
Matthew Draper
@matthewd
Apr 08 2016 04:17
:sunglasses:
Keenan Brock
@kbrock
Apr 08 2016 04:17
ManageIQ/manageiq#7813
please tear that one apart
Keenan Brock
@kbrock
Apr 08 2016 04:24
I'm fading - catch you tomorrow
Keenan Brock
@kbrock
Apr 08 2016 12:23
@matthewd what do you think?
  def select_or_detect?(key, value)
    if loaded?
      detect { |s| s.send(key) == value }
    else
      find_by(key => value)
    end
  end
not sure how to mix into all scopes - not sure if it is even a good idea
and the one use case I have is a spinoff of it - so it may not be a very good generalization either
Matthew Draper
@matthewd
Apr 08 2016 12:25
Yeah… the problem is that it doesn't generalize fully
(i.e., that's why find_by doesn't do it internally)
Keenan Brock
@kbrock
Apr 08 2016 12:25
yea - that is magic that you would expect from rails
guess you would want select_or_detect(hash) which starts to get very complex to support multiple keys and multiple values
k -thanks
Matthew Draper
@matthewd
Apr 08 2016 12:29
Hmm… it might be feasible to teach where to do in-ruby filtering if it knows how to (given the type of the value in the condition hash, and the type of the column)
Keenan Brock
@kbrock
Apr 08 2016 12:30
yea - probably for another day
more interested in ManageIQ/manageiq#7813
think I finally grok arel_attribute
looking at using that for virtual columns right about now
ugh - oops, I munged VirtualIncludes class :( needs to be pulled out
Matthew Draper
@matthewd
Apr 08 2016 12:33
I'm confused on that… why are we adding SQL stuff, and not using arel_attribute? :confused:
Keenan Brock
@kbrock
Apr 08 2016 12:35
first pass was to get working with rails 5
where should I store sql? a separate hash?
I JUST figured out are_attribute like 5 minutes ago
it is very simple. read through that PR ~2 times today and FINALLY got it
Matthew Draper
@matthewd
Apr 08 2016 12:36
Ah, okay
Keenan Brock
@kbrock
Apr 08 2016 12:36
ok, so a virtual delegate (to a real attribute) would be easier to start with?
Matthew Draper
@matthewd
Apr 08 2016 12:37
So, yes that's how we want to store the thing… but the thing we're storing should be an arel expression (or.. a proc that returns such?), instead of an SQL string
archived in that PR seems an ideal simple case
Keenan Brock
@kbrock
Apr 08 2016 12:38
think I'm going to split the pr into just the first commit
can you select(arel_table[:name], arel_table[:id])?
is there an easier way to tack on an inner join, like there is to tack on an outter join?
VmOrTemplate.joins(:ext_management_system).select("vms.name, ems.name")
VmOrTemplate.outter_joins(:ext_management_system).select("vms.name, ems.name") # <= I want
Matthew Draper
@matthewd
Apr 08 2016 12:40
I don't follow
Keenan Brock
@kbrock
Apr 08 2016 12:42
class VmOrTemplate 
  virtual_delegate :name, to: :ext_management_system, :prefix => true
end

VmOrTemplate.select(:name, :ext_management_system_name).first
VmOrTemplate.select(:name).sort(:ext_management_system_name).first
Matthew Draper
@matthewd
Apr 08 2016 12:42
That's… not the problem we were setting out to solve
Oh, yes, the latter is
But the former is not
Keenan Brock
@kbrock
Apr 08 2016 12:43
you saying I'm conflating? how dare you! ;)
yea, I believe you. just having trouble seeing the conflation there
Matthew Draper
@matthewd
Apr 08 2016 12:45
select is very different
Keenan Brock
@kbrock
Apr 08 2016 12:46
again, I take your word for it.
dreammers gonna dream
Keenan Brock
@kbrock
Apr 08 2016 12:53
omg - s/virtual_sql/arel_attribute/
Matthew Draper
@matthewd
Apr 08 2016 12:55
I think I'd s/virtual_sql/virtual_arel/, then define arel_attribute to use that
Better separates "new thing that stores a map of stuff" from "specific AR API we're overriding"
Keenan Brock
@kbrock
Apr 08 2016 12:56
+1
Matthew Draper
@matthewd
Apr 08 2016 12:57
(also, gives the right place to handle the proc call, if we go that way [I'm increasingly of the belief we should / will want to])
Keenan Brock
@kbrock
Apr 08 2016 12:57
whole way?
Keenan Brock
@kbrock
Apr 08 2016 13:12
@matthewd you mean to support select as well?
Matthew Draper
@matthewd
Apr 08 2016 13:13
No
Keenan Brock
@kbrock
Apr 08 2016 13:13
re inner_join, I was hoping that tacking on the sort would leverage the virtual_includes and auto add the inner_join
Matthew Draper
@matthewd
Apr 08 2016 13:19
Yeah, no, we can't do that
Keenan Brock
@kbrock
Apr 08 2016 13:20
can we write add_joins_for_columns(:name, :ext_management_system_name)?
Matthew Draper
@matthewd
Apr 08 2016 13:23
Do we need to?
Keenan Brock
@kbrock
Apr 08 2016 13:27
this is kinda tangental:
our reports
they allow the user to add any number of possibly columns to the query
so we include(all_possible_tables).references(all_possible_tables) in our code
Keenan Brock
@kbrock
Apr 08 2016 13:33
this makes our rbac count(*) even worse
Matthew Draper
@matthewd
Apr 08 2016 13:35
"this" = the includes/references thing?
Keenan Brock
@kbrock
Apr 08 2016 13:36
yes
includes/references requires count distinct
Matthew Draper
@matthewd
Apr 08 2016 13:37
Yeah, okay, leave that
As you said, tangent
Keenan Brock
@kbrock
Apr 08 2016 13:39
Sometimes I feel like a circle. there is a tangent everywhere
Keenan Brock
@kbrock
Apr 08 2016 13:51
@matthewd in ar_virtual:
_virtual_includes.keys.first.class == String? It looks like we may be missing a few to_s
aah - found it. thanks
at the beginning (line 99) there is a to_s - I was missing that in mine
Keenan Brock
@kbrock
Apr 08 2016 13:57
@matthewd I notice that _virtual_includeshas a lot of entries in it. is it suposed to be for all models, or unique per model?
Matthew Draper
@matthewd
Apr 08 2016 13:57
Per model
Keenan Brock
@kbrock
Apr 08 2016 13:58
I guess I'm working with VmOrTemplate
also, define_virtual_include name, uses if uses # adding the if uses -- right?
yea, that cleared it up quite a bit
Keenan Brock
@kbrock
Apr 08 2016 14:07
ok, checkpoint this is better and working
questions:
Ideas for providing better column definition here?
Matthew Draper
@matthewd
Apr 08 2016 14:08
.. that's still an SQL string
Keenan Brock
@kbrock
Apr 08 2016 14:09
correct
we want arel - right?
Matthew Draper
@matthewd
Apr 08 2016 14:10
Right
Keenan Brock
@kbrock
Apr 08 2016 14:10
so it takes a block?
should the block pass in an arel_table?
Also, adding an arel case statement rails/arel#400 looks a little daunting
Matthew Draper
@matthewd
Apr 08 2016 14:12
arel: -> t { t[:ems_id]…etc }
Keenan Brock
@kbrock
Apr 08 2016 14:12
that is nice
Matthew Draper
@matthewd
Apr 08 2016 14:12
If you must, you could arel: -> t { Arel.sql('this is cheating') }
Keenan Brock
@kbrock
Apr 08 2016 14:12
block.call(arel_table).to_s ? think I need to convert to a string? or I just leave as arel?
lol
Matthew Draper
@matthewd
Apr 08 2016 14:13
No string
Dude.
Keenan Brock
@kbrock
Apr 08 2016 14:13
k
lol
Matthew Draper
@matthewd
Apr 08 2016 14:13
Duude.
Keenan Brock
@kbrock
Apr 08 2016 14:13
lllllooooolllll
aah
Matthew Draper
@matthewd
Apr 08 2016 14:13
arel_attribute needs to return an arel node
Keenan Brock
@kbrock
Apr 08 2016 14:13
this is why:
no, arel_attribute returns arel - but we may need to convert that to sql in the short term
I HATE that LOWER - and it looks like they want more LOWER in the code
Matthew Draper
@matthewd
Apr 08 2016 14:16
We should have no reason to convert to SQL
Keenan Brock
@kbrock
Apr 08 2016 14:17
e.g.: ManageIQ/manageiq#7408 - current trend is to add LOWER everywhere in miq_expression
Matthew Draper
@matthewd
Apr 08 2016 14:17
Yes, that method will need to be fixed, though
Keenan Brock
@kbrock
Apr 08 2016 14:17
+1
ok, so you are saying to suck it up and remove the join(",")
you know this is potentially tricky. but if we stay in the order part, it can be smaller
Matthew Draper
@matthewd
Apr 08 2016 14:17
Another example of "every time we do our own thing, later changes get exponentially harder" :neutral_face:
Keenan Brock
@kbrock
Apr 08 2016 14:18
hey
you know how many times I've removed string sql
same boat / page / choir
Matthew Draper
@matthewd
Apr 08 2016 14:18
Yeah
Keenan Brock
@kbrock
Apr 08 2016 14:18
and to tell you the truth, I'm not much of an arel fan. I like active record hashes
Oleg Barenboim
@chessbyte
Apr 08 2016 14:19
but Arel gets you their nodes which can be processed separately
Keenan Brock
@kbrock
Apr 08 2016 14:19
more of an arel fan than raw sql. (and I'm VERY comfortable with sql)
I want to fix active record so we don't need to use arel
Matthew Draper
@matthewd
Apr 08 2016 14:20
That's not really feasible
Keenan Brock
@kbrock
Apr 08 2016 14:20
I'd prefer Vm.where(:id => 1...10) ; Vm.where(:id => 1..INFINITY)
100% != feasible
95% maybe
vs Vm.where(arel_table[:id].gt(1))
Matthew Draper
@matthewd
Apr 08 2016 14:21
Ummm… the former already works
Keenan Brock
@kbrock
Apr 08 2016 14:21
aah. right. not for dates
Matthew Draper
@matthewd
Apr 08 2016 14:21
But we're using arel here because it's the right tool for the job: AR condition hashes are never going to support arbitrary case statements
Keenan Brock
@kbrock
Apr 08 2016 14:26
@matthewd ok, drop col.arity and always pass arel_table to the block. this is going to be sql generation (not active record) and they'll need the table
In theory the user could use Vm.arel_table, but we'll pass it in to work better once arel_table leverages table alias more often.
A block can reference a foreign table (e.g.EMS.arel_table), and it will work in the short term. I worry that in the future, this will not be good and I'm not sure how to pass in all possible tables from the statement
Matthew Draper
@matthewd
Apr 08 2016 14:27
huh?
Sentence fragments confuse me :(
Keenan Brock
@kbrock
Apr 08 2016 14:28
or the fact that it is 4 completely different sentences?
Matthew Draper
@matthewd
Apr 08 2016 14:31
To not pass it in would be pretty rude
Keenan Brock
@kbrock
Apr 08 2016 14:31
+1
I crossed that line out :)
Matthew Draper
@matthewd
Apr 08 2016 14:31
arel_attribute gets given it; dropping it on the floor is more than an inconvenience for the callee
In my imagined plan, cross-table references are not a thing, and thus not a problem
Keenan Brock
@kbrock
Apr 08 2016 14:34
I'm conflating, but I want virtual delegates in here
seems most of these virtual attributes are joining to another table
Matthew Draper
@matthewd
Apr 08 2016 14:37
My intention is that those would be subqueries (with their own, localized, table references), and not use other joined stuff
In the most pathological cases, depending on how clever PG is feeling, it may be less efficient than it could be, but should be good enough
Keenan Brock
@kbrock
Apr 08 2016 14:39
no
sub queries and joins are very similar
Matthew Draper
@matthewd
Apr 08 2016 14:40
In the most pathological cases
Keenan Brock
@kbrock
Apr 08 2016 14:40
sometimes they even have the same explain
Matthew Draper
@matthewd
Apr 08 2016 14:40
Specifically, I'm thinking of cases where multiple references use the same related table
Keenan Brock
@kbrock
Apr 08 2016 14:40
+1
Matthew Draper
@matthewd
Apr 08 2016 14:41
There, using duplicative subqueries is likely to mean the DB does more work, but everything's relative.. and the relevant pages will be in memory, so.. meh :)
Keenan Brock
@kbrock
Apr 08 2016 14:43
choir...
my gut says that the last 10% will really need to use the same tables, not a sub query
can cross that bridge...
Matthew Draper
@matthewd
Apr 08 2016 14:45
My expectation is that if the subquery form isn't compatible, it's not a "pure" virtual attribute that we can hope to inline
But yeah, one thing at a time
Keenan Brock
@kbrock
Apr 08 2016 14:45
@matthewd api design: I'm thinking sort / select / ... all can use the same sql. do you think just passing a block is good enough, or you like the :select => -> t { t[:name] }
Matthew Draper
@matthewd
Apr 08 2016 14:46
I think I prefer to name it, because it doesn't feel sufficiently intrinsic to virtual_attribute to have its meaning implied by a bare block
Keenan Brock
@kbrock
Apr 08 2016 14:46
  virtual_attribute :archived, :boolean,
                    :arel => -> t { t[:ems_id].neq(nil).or(t[:storage_id].neq(nil)) }
k
Matthew Draper
@matthewd
Apr 08 2016 14:47
Likewise, name it :arel
Keenan Brock
@kbrock
Apr 08 2016 14:47
killed the case statement. this is working in select and order (even thought I know select is a different beast)
k
Matthew Draper
@matthewd
Apr 08 2016 14:48
select is dangerous
Keenan Brock
@kbrock
Apr 08 2016 14:48
  virtual_attribute :archived, :boolean, :select => [:ems_id, :storage_id], :uses => :ext_management_system,
                    :arel => -> t { t[:ems_id].neq(nil).or(t[:storage_id].neq(nil)) }
Matthew Draper
@matthewd
Apr 08 2016 14:49
virtual_attribute == "please note we've defined a custom method that you should treat as attribute-ish"
select == "define an AR built-in method to represent the DB-supplied column value"
Keenan Brock
@kbrock
Apr 08 2016 14:49
but if you look at a majority of them
they are delegating to another table
or calculating something (on the same table I think)
Matthew Draper
@matthewd
Apr 08 2016 14:50
What is the :select there?
Keenan Brock
@kbrock
Apr 08 2016 14:50
if you want to call this function in ruby, you need those columns to use it?
it is uses for columns :)
maybe?
Matthew Draper
@matthewd
Apr 08 2016 14:51
What maybe?
I thought we were trying to use the thing that already exists, to prove it is a useful solution to the problems it was designed to fix
Designing a solution to another problem at the same time seems very distracting
Keenan Brock
@kbrock
Apr 08 2016 14:52
need an emoji that means conflating
Matthew Draper
@matthewd
Apr 08 2016 14:58
I think that responsibility belongs elsewhere, fwiw… not up to the caller to care whether a given method is an association or a column; both should just go in :uses
But yeah, :clapper:
Keenan Brock
@kbrock
Apr 08 2016 14:59
:+1: s/conflate/ :clapper: /g
Keenan Brock
@kbrock
Apr 08 2016 15:56

ok, the ordering throws a slight wrinkle, but think I have it ironed out.
While I'm at it....

# old:
sql_col = [db_class.table_name, c].join(".")
# new:
sql_col = db_class.arel_table[c])

I'm concerned about just randomly adding arel_table references to a query. is this a problem?

I'm guessing you're suposed to get the arel_table reference from the SqlManager? Or am I trying to predect the future around arel_table and aliases and stuff?
@matthewd for tests, how do I take an arel node and convert into sql? (for easier comparisons)
Matthew Draper
@matthewd
Apr 08 2016 16:00
db_class.arel_attribute(c)
Keenan Brock
@kbrock
Apr 08 2016 16:00
aaaahhhhh!
is this going too far? should I keep as strings for now?
B) how do I arel -> String for comparison
Matthew Draper
@matthewd
Apr 08 2016 16:01
Does to_sql suffice?
Keenan Brock
@kbrock
Apr 08 2016 16:03
no
never mind
thought I tried that - perfect
:+1: thanks
Keenan Brock
@kbrock
Apr 08 2016 16:52
@matthewd found my to_sql confusion:
>VmOrTemplate.arel_table[:name].to_sql
NoMethodError: undefined method `to_sql' for #<Arel::Attributes::Attribute:0x007f915afc8148>
>VmOrTemplate.arel_table[:name].lower.to_sql
# => "LOWER(\"vms\".\"name\")"
Matthew Draper
@matthewd
Apr 08 2016 16:55
Ah, fair enough
As you're only using it in tests (so you know what the recipient should be), that should be fine, right? If you're expecting a plain column reference, you can just expect it with a direct equality
to_sql is only useful to avoid complexities in comparing a deeper nested structure, correct?
Keenan Brock
@kbrock
Apr 08 2016 16:56
+1
yes
agreed - avoid to_sql in real code
Matthew Draper
@matthewd
Apr 08 2016 16:57
Mind you, I'd argue any arel node's == should DTRT
Keenan Brock
@kbrock
Apr 08 2016 16:58
I had added concat
but I can't figure out how to make it work
Matthew Draper
@matthewd
Apr 08 2016 16:58
But if that's not the case, fixing it isn't likely to be the shortest path to achieving your goal ;)
Keenan Brock
@kbrock
Apr 08 2016 16:58
even in cd arel ; bundle console
:)
Keenan Brock
@kbrock
Apr 08 2016 19:35
@matthewd ok, this feels better: ManageIQ/manageiq#7813
Keenan Brock
@kbrock
Apr 08 2016 21:08
we are only supporting :arel => -> t { t[:name] } -- only blocks?
and it will NOT fall back to master
Keenan Brock
@kbrock
Apr 08 2016 21:53
@matthewd ok, think this is closer :)
Keenan Brock
@kbrock
Apr 08 2016 22:51
@matthewd Arel::Nodes::Case is not in arel 7.0
I'm having trouble casting a column to another value - e.g. an col::float- I'll see if that feature is in there. I can't use function as it isn't cast(col, float)