Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add support for pg geometric datatypes point and box #7324

Merged
merged 2 commits into from

9 participants

@MSch

@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?

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
@MSch
MSch added a note

this fails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove was assigned
@steveklabnik
Collaborator

This no longer merges cleanly.

@steveklabnik
Collaborator

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

@MSch

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

@MSch try pinging him on twitter.

/cc @tenderlove

@schneems
Collaborator

@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

@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
Owner

@MSch great! Let me know when you are done

@MSch

@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 https://github.com/rails/rails/blob/e209107a518c70c24d3d05ce246b8a79de9c7632/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb#L58) 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

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

@bai

Very interested in this one.

@rafaelfranca rafaelfranca commented on the diff
...b/active_record/connection_adapters/postgresql/oid.rb
@@ -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
@rafaelfranca Owner

Why it is instantiate latter?

@MSch
MSch added a note

see below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...b/active_record/connection_adapters/postgresql/oid.rb
@@ -63,6 +63,20 @@ def type_cast(value)
end
end
+ class Point < Vector
+ def initialize(delim, subtype)
+ super
+ end
+
+ def type_cast(value)
+ if String === value
+ ConnectionAdapters::PostgreSQLColumn.string_to_point value
@rafaelfranca Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...tive_record/connection_adapters/postgresql_adapter.rb
@@ -630,8 +630,15 @@ def initialize_type_map
# populate composite types
nodes.find_all { |row| OID::TYPE_MAP.key? row['typelem'].to_i }.each do |row|
- vector = OID::Vector.new row['typdelim'], OID::TYPE_MAP[row['typelem'].to_i]
- OID::TYPE_MAP[row['oid'].to_i] = vector
+ if OID.registered_type? row['typname']
+ # this composite type is explicitly registered
+ vector_class = OID::NAMES[row['typname']]
+ else
+ # use the default for composite types
+ vector_class = OID::Vector
+ end
+
+ OID::TYPE_MAP[row['oid'].to_i] = vector_class.new row['typdelim'], OID::TYPE_MAP[row['typelem'].to_i]
@rafaelfranca 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?

@MSch
MSch added a note

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)

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

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

@esad

+1

@MSch

@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?

@rafaelfranca

Seems good. Could you add a CHANGELOG entry?

...tive_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
@rafaelfranca Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...tive_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
@rafaelfranca Owner

Same here

@MSch
MSch added a note

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.

@rafaelfranca Owner

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

@rafaelfranca 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

@MSch
MSch added a note

Oh. I'll submit a second commit that fixes this then.

@MSch
MSch added a note

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

@rafaelfranca Owner

Great! Please do it

@rafaelfranca Owner

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

@rafaelfranca Owner

Done at be71b0e

@MSch
MSch added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca

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

@MSch

@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 rafaelfranca merged commit fc8411a into rails:master
@rafaelfranca

Thank you. :heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:

@MSch MSch deleted the unknown repository branch
@kewagi

I am in favor of liking this.

@schneems
Collaborator

Thanks for this code!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 activerecord/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* PostgreSQL geometric type point is supported by ActiveRecord. Fixes #7324.
+
+ *Martin Schuerrer*
+
* Add an `add_index` override in Postgresql adapter and MySQL adapter
to allow custom index type support. Fixes #6101.
View
11 activerecord/lib/active_record/connection_adapters/postgresql/cast.rb
@@ -2,6 +2,17 @@ module ActiveRecord
module ConnectionAdapters
class PostgreSQLColumn < Column
module Cast
+ def point_to_string(point)
+ "(#{point[0]},#{point[1]})"
+ end
+
+ def string_to_point(string)
+ if string[0] == '(' && string[-1] == ')'
+ string = string[1...-1]
+ end
+ string.split(',').map{ |v| Float(v) }
+ end
+
def string_to_time(string)
return string unless String === string
View
11 activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
@@ -63,6 +63,16 @@ def type_cast(value)
end
end
+ class Point < Type
+ def type_cast(value)
+ if String === value
+ ConnectionAdapters::PostgreSQLColumn.string_to_point value
+ else
+ value
+ end
+ end
+ end
+
class Array < Type
attr_reader :subtype
def initialize(subtype)
@@ -330,6 +340,7 @@ def self.registered_type?(name)
register_type 'time', OID::Time.new
register_type 'path', OID::Identity.new
+ register_type 'point', OID::Point.new
@rafaelfranca Owner

Why it is instantiate latter?

@MSch
MSch added a note

see below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
register_type 'polygon', OID::Identity.new
register_type 'circle', OID::Identity.new
register_type 'hstore', OID::Hstore.new
View
27 activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -28,10 +28,14 @@ def quote(value, column = nil) #:nodoc:
super
end
when Array
- if column.array
- "'#{PostgreSQLColumn.array_to_string(value, column, self)}'"
+ case sql_type
+ when 'point' then super(PostgreSQLColumn.point_to_string(value))
else
- super
+ if column.array
+ "'#{PostgreSQLColumn.array_to_string(value, column, self)}'"
+ else
+ super
+ end
end
when Hash
case sql_type
@@ -79,9 +83,10 @@ def quote(value, column = nil) #:nodoc:
def type_cast(value, column, array_member = false)
return super(value, column) unless column
+ sql_type = type_to_sql(column.type, column.limit, column.precision, column.scale)
case value
when Range
- return super(value, column) unless /range$/ =~ column.sql_type
+ return super(value, column) unless /range$/ =~ sql_type
PostgreSQLColumn.range_to_string(value)
when NilClass
if column.array && array_member
@@ -92,19 +97,23 @@ 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 sql_type
+ when 'point' then PostgreSQLColumn.point_to_string(value)
+ else
+ return super(value, column) unless column.array
+ PostgreSQLColumn.array_to_string(value, column, self)
+ end
when String
- return super(value, column) unless 'bytea' == column.sql_type
+ return super(value, column) unless 'bytea' == sql_type
{ :value => value, :format => 1 }
when Hash
- case column.sql_type
+ case sql_type
when 'hstore' then PostgreSQLColumn.hstore_to_string(value)
when 'json' then PostgreSQLColumn.json_to_string(value)
else super(value, column)
end
when IPAddr
- return super(value, column) unless ['inet','cidr'].include? column.sql_type
+ return super(value, column) unless ['inet','cidr'].include? sql_type
PostgreSQLColumn.cidr_to_string(value)
else
super(value, column)
View
9 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -711,7 +711,14 @@ def initialize_type_map
# populate composite types
nodes.find_all { |row| OID::TYPE_MAP.key? row['typelem'].to_i }.each do |row|
- vector = OID::Vector.new row['typdelim'], OID::TYPE_MAP[row['typelem'].to_i]
+ if OID.registered_type? row['typname']
+ # this composite type is explicitly registered
+ vector = OID::NAMES[row['typname']]
+ else
+ # use the default for composite types
+ vector = OID::Vector.new row['typdelim'], OID::TYPE_MAP[row['typelem'].to_i]
+ end
+
OID::TYPE_MAP[row['oid'].to_i] = vector
end
View
27 activerecord/test/cases/base_test.rb
@@ -840,7 +840,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.0, 6.1], h.a_point
assert_equal '[(2,3),(5.5,7)]', h.a_line_segment
assert_equal '(5.5,7),(2,3)', h.a_box # reordered to store upper right corner then bottom left corner
assert_equal '[(2,3),(5.5,7),(8.5,11)]', h.a_path
@@ -869,7 +869,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.0, 6.1], h.a_point
assert_equal '[(2,3),(5.5,7)]', h.a_line_segment
assert_equal '(5.5,7),(2,3)', h.a_box # reordered to store upper right corner then bottom left corner
assert_equal '((2,3),(5.5,7),(8.5,11))', h.a_path
@@ -880,6 +880,29 @@ def test_geometric_content
objs = Geometric.find_by_sql ["select isclosed(a_path) from geometrics where id = ?", g.id]
assert_equal true, objs[0].isclosed
+
+ # test native ruby formats when defining the geometric types
+ g = Geometric.new(
+ :a_point => [5.0, 6.1],
+ #:a_line => '((2.0, 3), (5.5, 7.0))' # line type is currently unsupported in postgresql
+ :a_line_segment => '((2.0, 3), (5.5, 7.0))',
+ :a_box => '(2.0, 3), (5.5, 7.0)',
+ :a_path => '((2.0, 3), (5.5, 7.0), (8.5, 11.0))', # ( ) is a closed path
+ :a_polygon => '2.0, 3, 5.5, 7.0, 8.5, 11.0',
+ :a_circle => '((5.3, 10.4), 2)'
+ )
+
+ assert g.save
+
+ # Reload and check that we have all the geometric attributes.
+ h = Geometric.find(g.id)
+
+ assert_equal [5.0, 6.1], h.a_point
+ assert_equal '[(2,3),(5.5,7)]', h.a_line_segment
+ assert_equal '(5.5,7),(2,3)', h.a_box # reordered to store upper right corner then bottom left corner
+ assert_equal '((2,3),(5.5,7),(8.5,11))', h.a_path
+ assert_equal '((2,3),(5.5,7),(8.5,11))', h.a_polygon
+ assert_equal '<(5.3,10.4),2>', h.a_circle
end
end
Something went wrong with that request. Please try again.