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

Add hstore parsing #1560

Merged
merged 4 commits into from Apr 2, 2014

Conversation

5 participants
@nunofgs
Contributor

nunofgs commented Mar 27, 2014

This is related to #1456.

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 27, 2014

Not sure how this allows you to query the values of an hstore field?

@nunofgs nunofgs changed the title from Add hstore querying to Add hstore parsing Mar 27, 2014

@fixe

This comment has been minimized.

Contributor

fixe commented Mar 27, 2014

This PR allows hstore fields to be parsed into objects instead of strings.

Consider the following hstore value, contained in the field attributes:

"city" => "Braga", "country" => "Portugal"

Here's how we are querying our data:

Project.find({ where: ['"attributes"->"city" = ?', 'Braga'] })
@janmeier

This comment has been minimized.

Member

janmeier commented Mar 27, 2014

Ah, then I understood the code correctly :)

Then i don't think the PR fixes 1456, which asks for a more sequelize'esque way of querying hstore, instead of building your own raw query strings.

The PR is still very valid though

@nunofgs

This comment has been minimized.

Contributor

nunofgs commented Mar 27, 2014

Ah, that's correct. Looks like I misunderstood #1456. I've edited the original description to reflect this.

@eav

This comment has been minimized.

eav commented Mar 28, 2014

I think it closes #1506

@@ -128,12 +128,20 @@ module.exports = (function() {
})
}
// Parse hstore fields.
// This cannot be done in the 'pg' lib because hstore is a UDT.
for (var key in rows[0]) {

This comment has been minimized.

@mickhansen

mickhansen Mar 28, 2014

Contributor

This seems like it would not work for findAll, mind adding a test for findAll and ensuring that works too?
Also this loop adds performance overhead, especially if done once per row - Perhaps we can optimize it so it's only run if there's a hstore in the query.

Also this will probably not work with hstore on includes - But we need to do some refactoring for that i guess.

This comment has been minimized.

@eav

eav Mar 28, 2014

you are right, findAll still has the issue, only first record is parsed.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 1, 2014

@nunofgs looks like you added support for findAll, is the PR good to go?

@nunofgs

This comment has been minimized.

Contributor

nunofgs commented Apr 1, 2014

@mickhansen I've just edited the commit with a bit of a performance improvement by checking if an hstore field exists on the target model.

Do you have any suggestions? If not, go ahead and merge :)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 1, 2014

@nunofgs i commented in the diff :)

@nunofgs

This comment has been minimized.

Contributor

nunofgs commented Apr 1, 2014

@mickhansen Ah, I agree with your suggestion. Check my latest change please.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 1, 2014

@nunofgs i added one final comment, please take a look.

Nuno Sousa
Add multiple row support to hstore parsing
* Improved performance by checking if query contains an hstore field.
@nunofgs

This comment has been minimized.

Contributor

nunofgs commented Apr 1, 2014

@mickhansen Done. Thank you for the suggestion!

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 2, 2014

Beautiful

mickhansen added a commit that referenced this pull request Apr 2, 2014

@mickhansen mickhansen merged commit 199ba5f into sequelize:master Apr 2, 2014

1 check passed

default The Travis CI build passed
Details

@nunofgs nunofgs deleted the seegno-forks:add-hstore-querying branch Apr 3, 2014

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