<GitHub100>
jruby/master 45141a7 Thomas E. Enebo: Whoopsie. I originally did not understand why we would initialize to UNDEF...
xardion has quit [Remote host closed the connection]
xardion has joined #jruby
<kares>
enebo: rdubya: hey! so I finally reached PG with the date/time issues (MySQL is fine)
<kares>
still do not see it as a thing to 'revert' much but smt necessary to be fixed due potential custom SELECT code
<kares>
really bumped into rdubya's Result refactorings ... and well its really unfinished with lots of duplication as I finally looked in detail
<kares>
not sure why we couldn't have delegated from there to the connection impl to handle the conversion
<kares>
+ the new conversion has slightly different conventions
<rdubya>
was mainly hoping we could get that stuff out of the adapter
<rdubya>
to clean them up some so they aren't as big
<kares>
looking into ways of fixing these but maybe I'll just wrap up
<kares>
rdubya: ah hey! well I do not get why did we ship it so WiP than
<rdubya>
we'd have to convert all the other ones to be able to clean it up much further
<kares>
not necessarily - is it different than the RubyJdbcConnection conversions?
<kares>
I suggested we delegate re-use those by default ... even if its different - maybe I am missing why it wasn't done so?
<rdubya>
most of them are basically the same but I cleaned up pieces that didn't look like they needed to be there anymore
<kares>
okay when I did the best I could so I leave it up to you guys than ...
<rdubya>
initially it was because of pulling them all out into other classes
<kares>
the date/time fixes should not be hard for PG - I just refuse to two them in 2 places :)
<kares>
* to do
<rdubya>
that simplified how much needed to be passed into each one but then we decided to move them back into the object itself
<rdubya>
i can take care of it, i have some other fixes that I'm wrapping up too
<kares>
okay great - so I think I'm done than
<kares>
if I may suggest - I think its really best to have that Result as thin as possible (get a RubyJdbcConn instance on creation) and delegate
<kares>
that's what I started doing but I just do not want to push it on you
<rdubya>
I'm open to it, what are the benefits of having them all in there?
<rdubya>
I think we could actually define them as static methods
<rdubya>
I don't think any of them use anything from the adapter
<rdubya>
except we need to be able to override them
<rdubya>
so i guess they couldn't be static
<enebo>
kares: rdubya: passing instance of rubyjdbcconn seems ok to me to reduce need for every adapter to have this result class
<kares>
its not about reduing the Result but the conversion duplication
<enebo>
kares: yeah understood
<kares>
which, as I noted, is slightly refactored in Result
<enebo>
kares: just saying there are two specific ways of reducing the duplication
<kares>
so its quite a show stopper for me to get things done there ... wouth going for a deep review as to why :)
<enebo>
kares: if all adapters used the result type then there would be no duplication as well but then it forces adapters to have that result type (which pg and mysql both have in the MRI native adapters)
<kares>
yep there are and to minimize internals leaking its best to keep conversion on place where you handle JDBC conn
<kares>
but that is just a suggestion :)
<kares>
there are times where you do not need to instantiate the Result piece
<enebo>
kares: well that is a good point
<kares>
its just a convenience wrapper as/when needed ... or at least seems to me
<enebo>
not that the duplication point was not
<enebo>
but the duplication I think was only potentially meant to be transitional
<rdubya>
is there a benefit of having all the adapters we support have a base result object that is the same?
<kares>
there's some other stuff we did not review
<kares>
like keeping/storing a class instance in static
<kares>
enebo: are there issues with that?
<enebo>
rdubya: yeah I don't know but for some things like inserts or pragmas we don't actually have a result but maybe a single converted value
<rdubya>
those don't create a result object
<enebo>
kares: yes. we should be storing nothing in static fields
<kares>
ok - thought so
<enebo>
for Rails it might not really matter a ton but who knows if someone has a custom embed app with AR it would definitely be an issue
<enebo>
so I guess if we have a static field we should make sure nothing which references Ruby.class is in it
<enebo>
historically it goes bad :)
<enebo>
I do not recall that in rdubya PR though
<rdubya>
that was in the original PR and I put a comment there asking if there was a better way to do it
<enebo>
oh ok I missed that
<kares>
yy I know I just wasn't sure if that still matters ... these days
<enebo>
I almost would say that is good advice for all Java apps though...if you use static only store really simple types like String
<enebo>
once you put a complex type in it you will probably create a leak some day when someone adds a new field to the object
<kares>
well I guess than its not that big of a deal for an 'internal' piece of Ruby class
<enebo>
so I agree with kares that passing in RubyJdbcConnection and having all conversion pretty much like it is now would eliminate duplication and in those cases where Result is not needed we won't have to stand up an instance just to convert a value
<enebo>
The other possible design choice would be to stand up a single converter instances when you originally load and then both connection and result could use that but I think that alternative design would just reduce class size (which I think was rdubya desire)
<enebo>
I don't really care a ton either way though.
<kares>
Result could/should just do jdbcToRuby if there isn't anything different (custom conversion) from the connection impl
<enebo>
kares: so to summarize the duplication cannot be eliminated with how the PR works without converting all adapters to do it. But we do some calls on connection which do not want to make a Result at all...and we would need to make a Result in this design to actually do the conversion. That about sum it up for you?
<rdubya>
what cases do we need to return results that we wouldn't want a result object?
<enebo>
rdubya: we have at least one call which just returns a count or index or something don't we?
<kares>
enebo: pretty much-have other concerns why I would want to to stay where is as some methods use the private internals of the connection impl
<enebo>
maybe that needs no conversion
<kares>
which got lost currently
<rdubya>
yeah but those don't run through the converters
<enebo>
kares: and you are worried if they are not private that someone will use them weirdly?
<enebo>
kares: or that it will limit ability to change internals perhaps
<kares>
nope- you just need to move a lot of stuff out which than gets harder to re-use ...
<enebo>
fwiw it seems like a lot of this debate is about doing conversion without standing up a result instance
<kares>
well it depends how much you would move over e.g. I stumbled upon no longer being able to switch raw/non-raw date-time returns
<kares>
they now work for MySQL but not for PG
<enebo>
I do not think jdbcToRuby need access anything in rubyjdbcconnection as private fields
<enebo>
but with that said why make a third type
<enebo>
but Result is not innately tied to data conversion in the same sense it is not tied to RubyJdbcConnection
<enebo>
in both cases they just have data they want converted
<kares>
enebo: consider you'll do the unwrapping of driver details e.g. for MySQL to handle byte[] + encoding
<kares>
you need the JDBC connection and store/cache some state
<kares>
e.g. the current encoding
<kares>
than cast down to some native stuff to get the actualy byte[]
<kares>
* native -> driver native
<enebo>
so we have some global state (in a limited sense) about our connection which we require to do conversion
<kares>
I can see that being much more noise and difficult to do in a separate class
<enebo>
and if that is external we would end up making something public and passing it around vs just simple private field access
<kares>
mostly a suggestion from me - I understood having as little as possible in native was desired and having more re-use with AR's .rb
<enebo>
kares: when you mentioned mysql you mean something we want to try in the future vs what we are doing now right?
<enebo>
I am just sniffing around in the code to see what private state we rely on now
<enebo>
I see several boolean flags in MySqlRubyJdbcConnection
<kares>
so I would not care much for proper Java design there :) but that is just me
<enebo>
kares: well as little as possible was an aspiration not a mandate
<kares>
enebo: wanted to try it but at this point I am out of time to do so right now
<enebo>
kares: I think if we double perf doing something and that means more support then it probably is worth it
<enebo>
kares: but at this point I would just like to have boring (even if slow) match with AR
<enebo>
Our big problem right now is not performance but having any support at all. So I look at this as more of a reboot where we start with least deviation and then we re-add what is good for us
<enebo>
good for us is very subjective
<enebo>
but if we get to the point where only issues reported ends up being we need to get faster I think that won't be a big problem
<kares>
enebo: okay than I do not think there's the luxury of refactoring much
<enebo>
kares: but I agree with you that if we need Connection data then it will all end up being public apis so another type can ask
<kares>
at this point I would hope for a release
<enebo>
kares: I think rdubya Result just reduces the mismatch with native pg
<enebo>
as it stands that is also true of mysql
<kares>
enebo: also I am not sure ... rdubya should do better but aren't there now 2 string-caches for Java->Ruby column names (with Result)?
<rdubya>
i can make it take the connection and call back to that
<rdubya>
there's 2 caches but all the pg stuff runs through the same cache as far as I can tell
<enebo>
rdubya: I think that eliminates one issue of duplication since maybe not all adapters will follow this result type
<enebo>
I feel out of depth in saying mysql and sqlite should have one
<enebo>
I did see mysql native does definitely rely on a very simple result object
<rdubya>
i think they should to match what AR has, postgres definitely uses one too
<enebo>
but that object has {size, fields, each} so only fields is something basically an array does not have
<rdubya>
which they unfortunatly don't share the same api :(
<enebo>
haha yeah
<rdubya>
the nice thing about the result is that we don't need the column data objects anymore
<enebo>
that is frustrating since they are tantalizingly close :)
<enebo>
rdubya: yeah true but we could pass those as params down rubyjdbcconnection but it is cleaner in result
<enebo>
I think it is funny we have three programmers and three opinions :)
<rdubya>
lol
<enebo>
but anyways sounds like passing connection into result to remove the duplication is a reasonable compromise right now
<enebo>
if in the near future I decide I also like result it might add pressure for the design to change or not
<enebo>
but I do not think we should be too worried right now. We made a massive amount of changes just removing the version specific stuff
<enebo>
Rome wasn't made in a day
<rdubya>
i'll try to get it taken care of but it will probably take a few days
<enebo>
rdubya: interestingly these methods are basically internal in both cases
<rdubya>
the converters?
<enebo>
yeah
<rdubya>
yeah they are basically internal
xardion has quit [Ping timeout: 256 seconds]
xardion has joined #jruby
cremes77 has joined #jruby
claudiuinberlin has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
ruurd has joined #jruby
ruurd has quit [Ping timeout: 248 seconds]
cremes77 has quit [Quit: cremes77]
cremes77 has joined #jruby
rdubya has quit [Read error: Connection reset by peer]
rdubya has joined #jruby
claudiuinberlin has joined #jruby
cremes77 has quit [Quit: cremes77]
Puffball_ has quit [Remote host closed the connection]