These are chat archives for rails-sqlserver/activerecord-sqlserver-adapter

19th
Jan 2015
Rails SQL Server
@rails-sqlserver
Jan 19 2015 21:21
Test
Sean Griffin
@sgrif
Jan 19 2015 21:26
Hello
Ken Collins
@metaskills
Jan 19 2015 21:26
Hey!
Just getting this setup… been on the list for awhile!
Sean Griffin
@sgrif
Jan 19 2015 21:27
:+1:
So I'm working on some refactoring of relation, and as part of that I'd end up moving the creation of arel bps to a class that doesn't have a reference to the connection adapter
Ken Collins
@metaskills
Jan 19 2015 21:27
Thanks for the review, finishing up someting else right now and shifting my brain gears.
Sean Griffin
@sgrif
Jan 19 2015 21:27
It sounds like that wouldn't break you, since it's only in where
No problem
Ken Collins
@metaskills
Jan 19 2015 21:27
Heck yea it would.
I solved an issue last night getting to the @engine
Sean Griffin
@sgrif
Jan 19 2015 21:28
Ha
Ken Collins
@metaskills
Jan 19 2015 21:28
rails-sqlserver/activerecord-sqlserver-adapter@5bc019e
Sean Griffin
@sgrif
Jan 19 2015 21:28
Care to elaborate?
Ken Collins
@metaskills
Jan 19 2015 21:28
Do different people will have different collations. So I just opted to put the config in a class setting vs instance.
So FYI, one thing that is NEW in this version of the adapter is that I am going to use OFFSET and FETCH.
That’s a big deal, the adapter has always been brittle using crazy hand crafted window functions.
Sean Griffin
@sgrif
Jan 19 2015 21:30
Interesting
Ken Collins
@metaskills
Jan 19 2015 21:30
I am going to piss off some people, but using a more modern ANSI standard will let us do better things.
That said, I have foudn that it has one draw back...
You can not use OFFSET and FETCH without an ORDER.
Back in the day, I faced this problem too with window functions, if you specified none, we did some work in our visitor to default to a PK.
Sean Griffin
@sgrif
Jan 19 2015 21:31
I think we tried to default in Rails proper at one point, as well
Ken Collins
@metaskills
Jan 19 2015 21:31
Just Sunday, I got the core adapter tests passing and started on the ActiveRecord failures.
Sean Griffin
@sgrif
Jan 19 2015 21:31
IIRC it got shot down because it can mess with indexing
Ken Collins
@metaskills
Jan 19 2015 21:31
Yea… it is in there, but only a few places.

first and #last, etc. Basics are in there.

WHO!
To big….
Sean Griffin
@sgrif
Jan 19 2015 21:32
Lol
Ken Collins
@metaskills
Jan 19 2015 21:32
Anyways...
Aside: I saw you a handful of times at RubyConf, should have :wave: and said hi.
These are janky.
But I need them…
Well, I could go back to TOP if someone does a limit with no offset.
But then I will be back into the same boat I was before… making select statement visitors based on top criteria.
Which breaks down really badly in complex associations and joins.
OFFSET and FETCH are clear winners, but to make FETCH happen, I need to always put in a ORDER.
SQL Server does not allow non-deterministic queries.
Sean Griffin
@sgrif
Jan 19 2015 21:35
What if Arel::Table knew the name of its primary key (optionally)?
Ken Collins
@metaskills
Jan 19 2015 21:36
TL;DR - As is now (with 100 tests failing) the only place I dip into @engine is those two places so to make FETCH happen.
DUDE!
That would be epic.
Sean Griffin
@sgrif
Jan 19 2015 21:36
Stop trying to make fetch happen
It's never going to happen
(Sorry)
Ken Collins
@metaskills
Jan 19 2015 21:36
Hahahahahahahahahah
So… last night.
I was making a Minivan test pass.
This was in my notes to you...
Minivan.arel_table

# => #<Arel::Table:0x007fabc5d18c10
#  @aliases=[],
#  @columns=nil,
#  @engine=Minivan(minivan_id: string, name: string, speedometer_id: string, color: string),
#  @name="minivans",
#  @primary_key=nil,
#  @table_alias=nil>
I wanted to make sure that was right.
And I did some investigation… and it seems to be normal.
When there is no IDENTITY column...
And a key is assigned. Arel knows not of it.
So yea, if Arel::Table was configured with what Rails new.
Then I would be in a great spot.
Sean Griffin
@sgrif
Jan 19 2015 21:39
I'm actually not sure where that ivar is coming from
Oh
rails/arel@ec08368
Lol
But yeah, I think it'd be fine to just be a constructor param and have rails pass it in
The object is really simple. Rails knows more than it is given.
Sean Griffin
@sgrif
Jan 19 2015 21:42
Well that is intentional for the most part. Most arel objects are intended to be dumb structs
A lot of this falls into drawing that line between information that should exist on the AR class, and information that should exist at the connection adapter level. What I'd like to have happen is that Arel never asks for information, but is instead given the information it needs
e.g. types
Ken Collins
@metaskills
Jan 19 2015 21:43
I can agree with that. It’s needs more info.
Sean Griffin
@sgrif
Jan 19 2015 21:45
Yeah, primary key is definitely something I'd be fine with it knowing and that we can easily give it
Right now my current driver is making the attributes api public and moving it up to Active Model
Both of those require the implementation to stop overriding columns and columns_hash, which means separating type casting from columns
That requires removing type casting from arel (done) for the quote case, and changing how bind values are represented for the type_cast case
And that has opened a whole can of worms, and is leading me down a refactoring path that I've been meaning to investigate for a while
Because most of AR cares about them internally, how they're represented, how to add them, etc, and about once every week or two a bug comes up because some part of AR got it wrong and we're losing bound params somewhere
Ken Collins
@metaskills
Jan 19 2015 21:54
Interesting.
When I picked up the 4.1 work that others did, no one noticed the f*cked up and lost all prepared statements in the adapter.
I have some other notes… the quoting stuff...
But first… a DB raw connection aside.
When I authored TinyTDS, I made it return the proper ruby primitive for every object in the DB.
One thing that bugs me about ActiveRecord is that it assumes strings and casting, likely to PG gem.
Sean Griffin
@sgrif
Jan 19 2015 21:56
Right, two of the dbs will return primitives, two don't
Ken Collins
@metaskills
Jan 19 2015 21:56
Dont get me wrong, these new Type objects are crazy cool, and needed for other places things like pre DB validations and input.
From users.
That said, that is what this is for...
This allows an easy single config so I can still offer the crappy ODBC connection mode.
So when users connect, I can let ActiveRecord the legwork.
Trivia topic, just wanted to share.
About quoting...
SQL Server can not accept an ISO8601 string for every date/time data type.
Same could be technically made for every string type.
N’string’ is technically for national (unicode’ish) columns.
But I have to do in mass.
I have had many issues where people said, the adapter is crusing my indexes because of the quoting.
Non-national types will not leverage the index when N’string’ quoting is used.
Sean Griffin
@sgrif
Jan 19 2015 21:59
Yeah, I'm leaning towards passing in the column name
Ken Collins
@metaskills
Jan 19 2015 21:59
I was really hoping a stonger column to cast type helps this.
Sean Griffin
@sgrif
Jan 19 2015 22:00
Because the types should not need to know whether or not their primitive has different quoting behavior for a given DB type
Ken Collins
@metaskills
Jan 19 2015 22:00
When users of a different langauge use SQL Server, I memoize their DB’s date format %m %Y %d could be differnt order.
They should not need, but I think they kind of do.
if I hear you right. For example...
This was a big pain point...
Sean Griffin
@sgrif
Jan 19 2015 22:01
Well quoting is definitely a DB concern
Ken Collins
@metaskills
Jan 19 2015 22:01
Our date type… to work with different languages and the fact that to_s(:db) is not global...
Sean Griffin
@sgrif
Jan 19 2015 22:01
Basically what I'm imagining is that type_cast_for_database returns some sort of standard primitive
And methods like quote become the translation layer from that, to what's needed for the query
Sean Griffin
@sgrif
Jan 19 2015 22:02
At which point it's safe to fork based on the sql type, because the type object has done everything it's ever going to
And while users can/will introduce new types (e.g. serialize) on the ruby side, they won't be doing so on the db side
Ken Collins
@metaskills
Jan 19 2015 22:03
Sounds good.
Sean Griffin
@sgrif
Jan 19 2015 22:03
Yeah, to_s(:db) on anything makes 0 sense IMO
Ken Collins
@metaskills
Jan 19 2015 22:03
I would have to code to it to see if pracitce agreed.
```ruby
def with_sqlserver_db_date_formats
  old_db_format_date = Date::DATE_FORMATS[:db]
  old_db_format_time = Time::DATE_FORMATS[:db]
  date_format = Date::DATE_FORMATS[:_sqlserver_dateformat]
  Date::DATE_FOTRMATS[:db] = "#{date_format}"
  Time::DATE_FORMATS[:db] = "#{date_format} %H:%M:%S"
  yield
ensure
  Date::DATE_FORMATS[:db] = old_db_format_date
  Time::DATE_FORMATS[:db] = old_db_format_time
end
Ken Collins
@metaskills
Jan 19 2015 22:04
Moar :DB
Sean Griffin
@sgrif
Jan 19 2015 22:04
I'm sure there are absolutely no SQL injection vulnerabilities there
Ken Collins
@metaskills
Jan 19 2015 22:05
Oh… interesting.
I need to make a note of that.
FYI, eating dinner soon.
Sean Griffin
@sgrif
Jan 19 2015 22:06
Basically the removal of column from quote and type_cast was to make it clear in the code that the DB adapter was not the right place to handle richer types. I think that's been sufficiently handled at this point, and you make a good point about things like collation, different quoting mechanisms, etc
Ken Collins
@metaskills
Jan 19 2015 22:06
I’ll be here all night. I need to finish the 4.2 work, I got an OS sponsor for the project.
Sean Griffin
@sgrif
Jan 19 2015 22:06
(By richer I mean ruby land conversions, etc)
Ken Collins
@metaskills
Jan 19 2015 22:06
Cool, thanks for letting me bend your ear.
Agreed.
Sean Griffin
@sgrif
Jan 19 2015 22:06
Of course, always happy to discuss. :)
Ken Collins
@metaskills
Jan 19 2015 22:07
I really do appreciate it. This was fun too, finally got Gitter back up and running. Used it for thoughtbot/bourbon awhile back and always wanted to get back in.
Sean Griffin
@sgrif
Jan 19 2015 22:07
If you get a chance to investigate that substitute_at thing, please let me know. It seems to me like it could never be hit from where because if it did everything would break
Ken Collins
@metaskills
Jan 19 2015 22:07
I will! That may indeed be needed, I was reading up on use cases for that datatype too.
Sean Griffin
@sgrif
Jan 19 2015 22:20
The issue is if I did where(foo: "bar"), and substitute_at returned nil, the number of bind params supplied to the query would not match up with the number of substitutes (aside from the fact that the value being queried would be lost)
Sorry if that message appeared twice I can't tell if it failed to send the first time