Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not query database for non-spatial tables #125

Merged
merged 4 commits into from
Jun 11, 2014
Merged

Do not query database for non-spatial tables #125

merged 4 commits into from
Jun 11, 2014

Conversation

teeparham
Copy link
Member

This PR avoids unnecessary SQL queries of geometry_columns for tables that do not have any spatial columns.

Fix #71

  • Add a private SpatialColumnInfo class to manage the spatial column
    info query and memoization.
  • Add mocha to test expectations.

@teeparham
Do not query database for non-spatial tables
06ea213 
Fix #71

* Add a private SpatialColumnInfo class to manage the spatial column
info query and memoization.
* Add mocha to test expectations.
@teeparham teeparham mentioned this pull request Jun 9, 2014
Closed
@srt32
Copy link
Member

srt32 commented Jun 9, 2014

I believe the SpatialColumnInfo class is not actually private. It can be accessed from outside of MainAdapter by referring to its inheritance chain. For example, from inside of ConnectionAdapters::PostGISAdapter::SpatialColumn, running MainAdapter::SpatialColumnInfo returns the class, so it is defined. Perhaps SpatialColumnInfo could be a module and it could be mixed into MainAdapter only. In this way, MainAdapter stays a bit smaller and it's potentially clearer what's going on. Does that idea make sense?

teeparham added 2 commits June 10, 2014 09:34
@teeparham
Move SpatialColumnInfo to a separate file
736c5aa 
* It’s now a proper class. Private classes are not a thing.
* Remove unnecessary ‘.rb’ from requires
@teeparham
Test spatial column info caching
471690e
@teeparham
Copy link
Member Author

@srt32 thanks for reviewing. Good point. I made SpatialColumnInfo a public class.

I think this is ready for merge. I'll leave it open for review/comments for another day or so.

srt32
srt32 reviewed Jun 11, 2014
View reviewed changes
lib/active_record/connection_adapters/postgis_adapter/spatial_column_info.rb
module PostGISAdapter
# Do spatial sql queries for column info and memoize that info.
class SpatialColumnInfo
attr_accessor :adapter, :table_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these attr_accessor's become attr_readers? Either way, I believe they could also be privatized as they are not used by anything outside of the class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed them in the latest commit.

teeparham added a commit that referenced this pull request Jun 11, 2014
@teeparham
Merge pull request #125 from rgeo/spatial_column_info
f352844 
Do not query database for non-spatial tables
@teeparham teeparham merged commit f352844 into master Jun 11, 2014
@teeparham teeparham deleted the spatial_column_info branch June 11, 2014 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Weird geometry lookups on queries
2 participants
@teeparham @srt32