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.

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

Added array support to postgres adapter #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added array support to postgres adapter #65

wants to merge 2 commits into from

Conversation

AnalogJ
Copy link

@AnalogJ AnalogJ commented May 31, 2014

I added array support to the adapter. Querying arrays works as well.

@AnalogJ
Copy link
Author

AnalogJ commented May 31, 2014

possible solution to #36

@tjwebb
Copy link
Contributor

tjwebb commented Aug 28, 2014

Could you merge in master? I'd love to test this and pull it in

@AnalogJ
Copy link
Author

AnalogJ commented Aug 28, 2014

While I would love to have you merge my changes into the sails-postgresql, they won't work without my modifications to waterline-sequel - https://github.com/AnalogJ/waterline-sequel.git

Unfortunately those changes are very specific to postgresql, and waterline-sequel seems like it would be used by multiple adapters. If I'm wrong about that, I can make a pull request there as well.

@kevinburkeshyp
Copy link

Any update here?

@@ -19,8 +19,8 @@
"lodash": "~2.4.1",
"async": "~0.9.0",
"waterline-errors": "~0.10.0",
"waterline-sequel": "~0.0.2",
"waterline-cursor": "~0.0.3"
"waterline-sequel": "git://github.com/AnalogJ/waterline-sequel.git",
Copy link
Member

Choose a reason for hiding this comment

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

This is dangerous as it exposes users to waterline-sequel edge version.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • "waterline-sequel": "git://github.com/AnalogJ/waterline-sequel.git",

@AnalogJ Yea this is never going to happen. Just submit a PR to waterline-sequel and we'll merge the work in there. If you already have, then we'll take a look.

@dmarcelino
Copy link
Member

Unfortunately those changes are very specific to postgresql, and waterline-sequel seems like it would be used by multiple adapters. If I'm wrong about that, I can make a pull request there as well.

Waterline-sequel is used by: machinepack-mysql, machinepack-postgresql, sails-cassandra, sails-mysql, sails-oracledb and sails-postgresql.

We shouldn't change the waterline-sequel dependency to git version as that has the potential of exposing users to bugs. Not only that it makes it impossible to know which versions of waterline-postgresql are compatible with which versions of waterline-sequel.

@AnalogJ
Copy link
Author

AnalogJ commented Apr 1, 2015

@dmarcelino Yeah this PR is almost a year old. At the time I wrote it, there were a few issues talking about properly handling arrays in the postgres adapter. It should definitely NOT be merged as is.
The commit doesn't point to edge balderdashy/waterline-sequel it actually points to my branch AnalogJ/waterline-sequel.

More than getting this commit merged, I wanted to get the postgres adapter developers to take a look at my changes (including the ones in AnalogJ/waterline-sequel) and determine if they could be pulled into the repo. They'll be way out of date by now.

@dmarcelino
Copy link
Member

I see. @tjwebb, @particlebanana, is there any updates regarding handling arrays in postgres?

@particlebanana
Copy link
Contributor

The main issue with this last time I looked was that you needed to specify an array type eg: text, integer, etc. So just defining type array isn't enough. I haven't looked back at it in awhile though. I also remember something about having to declare a size. I don't think the size is actually a restriction though looking at the docs.

Array attributes aren't something I've ever used so if you guys think it's all good we can figure out how to get it in WL sequel.

@kevinburkeshyp
Copy link

It would be super useful... right now we're storing arrays as JSON blobs,
however, Postgres support for querying JSON blobs with an array at the top
level was only added in 9.4: http://stackoverflow.com/q/27912776/329700, so
any time we want to, for example, test for item membership in a array
stored as JSON, we need to fetch the whole thing into memory and then check
whether the item is in the array.

As an example use case, consider a list of zip codes that are supported
for your service.

On Wed, Apr 1, 2015 at 1:17 PM, Cody Stoltman [email protected]
wrote:

The main issue with this last time I looked was that you needed to specify
an array type eg: text, integer, etc. So just defining type array isn't
enough. I haven't looked back at it in awhile though. I also remember
something about having to declare a size. I don't think the size is
actually a restriction though looking at the docs.

Array attributes aren't something I've ever used so if you guys think it's
all good we can figure out how to get it in WL sequel.


Reply to this email directly or view it on GitHub
#65 (comment)
.

kevin

@particlebanana
Copy link
Contributor

@kevinburkeshyp @AnalogJ @dmarcelino are there any workarounds to this in the short term? Are you writing custom sql queries for this in the meantime? If it's only an issue of querying then a custom sql query using .query is ok for the short term. I don't know how writes handle this though.

This stuff is where wl-sequel begins to break down. As far as I know no other sql databases that use wl-sequel support json/array querying like this. It might be advantageous to add a lib for creating queries on json/array data directly in the postgres adapter.

@kevinburkeshyp
Copy link

We can't write custom queries, the only ways to deal with this is are a) pull the entire field up and sort/test membership in memory, or b) upgrade to Postgres 9.4. Upgrading would give us the ability to query JSON objects outside of WL (or by writing a custom SQL query to match), obv. wouldn't change anything inside the ORM.

@particlebanana
Copy link
Contributor

Hmm no custom queries is tricky. We added the .query method to the sql adapters specifically for cases like this. If writes to arrays work fine (have not tested at all) then you could just use a custom query for when you needed to check membership and then the ORM for writes/updates etc.

Are there any libraries that handle the postgres array/json query generation? I looked at Knex but it doesn't seem to have anything. It's a very postgres specific thing so I'd like to bolt on something to the adapter that already handles this and just have it as an alternative to find. If we have to roll our own that's a lot bigger task.

@carsonwright
Copy link

I know this is really really old, but I can't find your other dependent branch, I'm trying to get this all working for a project and if I could get my hands on the right waterline sql lib that would be awesome

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

Successfully merging this pull request may close these issues.

6 participants