Add support for pg geometric datatypes point and box #7324

Merged
merged 2 commits into from Mar 25, 2013

Conversation

Projects
None yet
9 participants
@MSch
Contributor

MSch commented Aug 10, 2012

@tenderlove https://twitter.com/tenderlove/statuses/231807764907819008

Today I looked into adding support for the postgresql geo types. Everything went well but I didn't manage to get AR to type cast pg's string representation into an array. (See the failing base_test.rb)

Got a pointer for me?

@MSch

View changes

activerecord/test/cases/base_test.rb
@@ -1174,7 +1174,7 @@ def test_geometric_content
# Reload and check that we have all the geometric attributes.
h = Geometric.find(g.id)
- assert_equal '(5,6.1)', h.a_point
+ assert_equal [5,6.1], h.a_point

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Aug 10, 2012

Contributor

this fails

@MSch

MSch Aug 10, 2012

Contributor

this fails

@ghost ghost assigned tenderlove Aug 10, 2012

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Sep 21, 2012

Member

This no longer merges cleanly.

Member

steveklabnik commented Sep 21, 2012

This no longer merges cleanly.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Nov 17, 2012

Member

@MSch ping! Any interest in keeping this pull request up to date?

Member

steveklabnik commented Nov 17, 2012

@MSch ping! Any interest in keeping this pull request up to date?

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Nov 17, 2012

Contributor

Would love to but I'm stuck with the data type / oid conversions. I'd need some input by someone knowledable (e.g. @tenderlove) on how to best make those conversions actually work in both directions.

Contributor

MSch commented Nov 17, 2012

Would love to but I'm stuck with the data type / oid conversions. I'd need some input by someone knowledable (e.g. @tenderlove) on how to best make those conversions actually work in both directions.

@guilleiguaran

This comment has been minimized.

Show comment Hide comment
@guilleiguaran

guilleiguaran Dec 28, 2012

Owner

@MSch try pinging him on twitter.

/cc @tenderlove

Owner

guilleiguaran commented Dec 28, 2012

@MSch try pinging him on twitter.

/cc @tenderlove

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Jan 2, 2013

Member

@MSch can you please explain the problem you're having starting from the beginning and pretending I have no prior knowldge of the issue? @tenderlove and the other AR maintainers are notoriously overwhelmed with help requests, the more info you can give me and them the better.

Member

schneems commented Jan 2, 2013

@MSch can you please explain the problem you're having starting from the beginning and pretending I have no prior knowldge of the issue? @tenderlove and the other AR maintainers are notoriously overwhelmed with help requests, the more info you can give me and them the better.

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Jan 9, 2013

Contributor

@schneems Thanks I'll do that. And thanks for offering! Had a busy last week, so sorry for the late answer:

Postgres has a type point which is a tuple of two floats. I want to add native support to ActiveRecord for this type.

When ActiveRecord queries Postgres about the type of such a point column it doesn't return "point" but rather "this is a composite type made up of two floats" (OIDs of data types).

I looked through the then current AR code to figure out how to hook into only that and then convert PG point <--> Ruby Array but for some reason I couldn't get it to work.

I've seen that my pull request doesn't merge cleanly any more, so maybe something has changed. I'll try again to make this work (since in principle it should be easy IMO) and will come back with some concrete question once I've hit a roadblock.

Contributor

MSch commented Jan 9, 2013

@schneems Thanks I'll do that. And thanks for offering! Had a busy last week, so sorry for the late answer:

Postgres has a type point which is a tuple of two floats. I want to add native support to ActiveRecord for this type.

When ActiveRecord queries Postgres about the type of such a point column it doesn't return "point" but rather "this is a composite type made up of two floats" (OIDs of data types).

I looked through the then current AR code to figure out how to hook into only that and then convert PG point <--> Ruby Array but for some reason I couldn't get it to work.

I've seen that my pull request doesn't merge cleanly any more, so maybe something has changed. I'll try again to make this work (since in principle it should be easy IMO) and will come back with some concrete question once I've hit a roadblock.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 9, 2013

Owner

@MSch great! Let me know when you are done

Owner

rafaelfranca commented Jan 9, 2013

@MSch great! Let me know when you are done

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Jan 9, 2013

Contributor

@rafaelfranca This time round I didn't get stuck with the vector conversion like last time.

This actually works, but I'm not happy with some of the code. I'm not sure if it wouldn't be better to generally convert all vectors into ruby arrays (like described here

# FIXME: this should probably split on +delim+ and use +subtype+
) or not. OTOH I'd actually love to convert points, uuids, etc. into specialized ruby types that closer mirror Postgres semantics. But that's another PR.

Comments anyone?

Contributor

MSch commented Jan 9, 2013

@rafaelfranca This time round I didn't get stuck with the vector conversion like last time.

This actually works, but I'm not happy with some of the code. I'm not sure if it wouldn't be better to generally convert all vectors into ruby arrays (like described here

# FIXME: this should probably split on +delim+ and use +subtype+
) or not. OTOH I'd actually love to convert points, uuids, etc. into specialized ruby types that closer mirror Postgres semantics. But that's another PR.

Comments anyone?

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Jan 16, 2013

Contributor

@rafaelfranca ping! it's been a week, i'm anxious :) got any input for me?

Contributor

MSch commented Jan 16, 2013

@rafaelfranca ping! it's been a week, i'm anxious :) got any input for me?

@bai

This comment has been minimized.

Show comment Hide comment
@bai

bai Jan 16, 2013

Very interested in this one.

bai commented Jan 16, 2013

Very interested in this one.

@@ -272,6 +286,7 @@ def self.registered_type?(name)
register_type 'time', OID::Time.new
register_type 'path', OID::Identity.new
+ register_type 'point', OID::Point

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

Why it is instantiate latter?

@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

Why it is instantiate latter?

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Jan 16, 2013

Contributor

see below.

@MSch

MSch Jan 16, 2013

Contributor

see below.

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
+
+ def type_cast(value)
+ if String === value
+ ConnectionAdapters::PostgreSQLColumn.string_to_point value

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

Because we have to allow people to create their own OID class

@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

Because we have to allow people to create their own OID class

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+ vector_class = OID::Vector
+ end
+
+ OID::TYPE_MAP[row['oid'].to_i] = vector_class.new row['typdelim'], OID::TYPE_MAP[row['typelem'].to_i]

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

@MSch said: Since I want to pass typdelim and typelem to the vector instance I have to instantiate it here and not already in register_type.

Humm I'm not sure about this one. @tenderlove?

@rafaelfranca

rafaelfranca Jan 16, 2013

Owner

@MSch said: Since I want to pass typdelim and typelem to the vector instance I have to instantiate it here and not already in register_type.

Humm I'm not sure about this one. @tenderlove?

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Jan 16, 2013

Contributor

Another approach would be to hardcode special casing for points here.

Anyway, both approaches are just workarounds because I haven't implemented perfect support for OID::Vector (which is a bit harder)

@MSch

MSch Jan 16, 2013

Contributor

Another approach would be to hardcode special casing for points here.

Anyway, both approaches are just workarounds because I haven't implemented perfect support for OID::Vector (which is a bit harder)

@bai

This comment has been minimized.

Show comment Hide comment
@bai

bai Jan 26, 2013

I've seen postgres range support PR has been merged, any decisions on this one? /cc @tenderlove

bai commented Jan 26, 2013

I've seen postgres range support PR has been merged, any decisions on this one? /cc @tenderlove

@esad

This comment has been minimized.

Show comment Hide comment
@esad

esad Jan 30, 2013

Contributor

+1

Contributor

esad commented Jan 30, 2013

+1

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 7, 2013

Contributor

@rafaelfranca @tenderlove I've fixed the issue with instantiating OID:Point later.

Anything else that needs to be done so that this can be merged?

Contributor

MSch commented Mar 7, 2013

@rafaelfranca @tenderlove I've fixed the issue with instantiating OID:Point later.

Anything else that needs to be done so that this can be merged?

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 25, 2013

Contributor

@rafaelfranca ping :)

Contributor

MSch commented Mar 25, 2013

@rafaelfranca ping :)

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Seems good. Could you add a CHANGELOG entry?

Owner

rafaelfranca commented Mar 25, 2013

Seems good. Could you add a CHANGELOG entry?

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -26,10 +26,14 @@ def quote(value, column = nil) #:nodoc:
super
end
when Array
- if column.array
- "'#{PostgreSQLColumn.array_to_string(value, column, self)}'"
+ case column.sql_type

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

We have to change it to sql_type since this method was removed from Column. See 4b4c8bd

@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

We have to change it to sql_type since this method was removed from Column. See 4b4c8bd

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -87,8 +91,12 @@ def type_cast(value, column, array_member = false)
super(value, column)
end
when Array
- return super(value, column) unless column.array
- PostgreSQLColumn.array_to_string(value, column, self)
+ case column.sql_type

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Same here

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 25, 2013

Contributor

Other parts of this method still use column.sql_type. I can switch them all in a separate commit if that's the plan anyhow.

@MSch

MSch Mar 25, 2013

Contributor

Other parts of this method still use column.sql_type. I can switch them all in a separate commit if that's the plan anyhow.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

I don't know how this works since that method was removed.

@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

I don't know how this works since that method was removed.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

master is passing right now but it is because we don't have tests to this. Column#sql_type doesn't exist anymore. See 7a872e9#L0L23

@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

master is passing right now but it is because we don't have tests to this. Column#sql_type doesn't exist anymore. See 7a872e9#L0L23

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 25, 2013

Contributor

Done. I'll work on adding a test case for this next, if no one (from the core team) is on that already?

@MSch

MSch Mar 25, 2013

Contributor

Done. I'll work on adding a test case for this next, if no one (from the core team) is on that already?

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Great! Please do it

@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Great! Please do it

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

@tenderlove just pointed me that we have another sql_type method, and it is the used here. I'll change it.

@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

@tenderlove just pointed me that we have another sql_type method, and it is the used here. I'll change it.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Done at be71b0e

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 25, 2013

Contributor

Oh, ok that explains why I couldn't get anything to fail :)

On Mon, Mar 25, 2013 at 8:03 PM, Rafael Mendonça França <
notifications@github.com> wrote:

In
activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb:

@@ -87,8 +91,12 @@ def type_cast(value, column, array_member = false)
super(value, column)
end
when Array

  •        return super(value, column) unless column.array
    
  •        PostgreSQLColumn.array_to_string(value, column, self)
    
  •        case column.sql_type
    

Done at be71b0ehttps://github.com/rails/rails/commit/be71b0ecbe4b6f2416296dd0c120b652151ebcaa


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7324/files#r3518605
.

@MSch

MSch Mar 25, 2013

Contributor

Oh, ok that explains why I couldn't get anything to fail :)

On Mon, Mar 25, 2013 at 8:03 PM, Rafael Mendonça França <
notifications@github.com> wrote:

In
activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb:

@@ -87,8 +91,12 @@ def type_cast(value, column, array_member = false)
super(value, column)
end
when Array

  •        return super(value, column) unless column.array
    
  •        PostgreSQLColumn.array_to_string(value, column, self)
    
  •        case column.sql_type
    

Done at be71b0ehttps://github.com/rails/rails/commit/be71b0ecbe4b6f2416296dd0c120b652151ebcaa


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7324/files#r3518605
.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Please rebase against master to get the last code changes on the schema methods and see if the tests are passing

Owner

rafaelfranca commented Mar 25, 2013

Please rebase against master to get the last code changes on the schema methods and see if the tests are passing

@MSch

This comment has been minimized.

Show comment Hide comment
@MSch

MSch Mar 25, 2013

Contributor

@rafaelfranca Added CHANGELOG entry. Changed to sql_type, but only in quote, not in type_cast since we're still using column.sql_type there and the tests pass.

Contributor

MSch commented Mar 25, 2013

@rafaelfranca Added CHANGELOG entry. Changed to sql_type, but only in quote, not in type_cast since we're still using column.sql_type there and the tests pass.

rafaelfranca added a commit that referenced this pull request Mar 25, 2013

Merge pull request #7324 from MSch/pg-geometric
Add support for pg geometric datatypes point and box

@rafaelfranca rafaelfranca merged commit fc8411a into rails:master Mar 25, 2013

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Mar 25, 2013

Owner

Thank you. ❤️ 💚 💙 💛 💜

Owner

rafaelfranca commented Mar 25, 2013

Thank you. ❤️ 💚 💙 💛 💜

@MSch MSch deleted the MSch:pg-geometric branch Mar 25, 2013

@kewagi

This comment has been minimized.

Show comment Hide comment
@kewagi

kewagi Mar 27, 2013

I am in favor of liking this.

kewagi commented Mar 27, 2013

I am in favor of liking this.

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Mar 27, 2013

Member

Thanks for this code!!!

Member

schneems commented Mar 27, 2013

Thanks for this code!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment