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

Feature/mango #143

Merged
merged 14 commits into from
Jun 25, 2019
Merged

Feature/mango #143

merged 14 commits into from
Jun 25, 2019

Conversation

ErickFabian
Copy link
Contributor

@ErickFabian ErickFabian commented Jun 20, 2019

Adds base mango querys for dolly documents

Pending Work:

Mango query indexes

Working example

> query = {and: [{ _id: { eq: 'user/+04diego@gmail.com' } } , { first_name: { eq: 'Diego'}} ]}
=> {:and=>[{:_id=>{:eq=>"user/+04diego@gmail.com"}}, {:first_name=>{:eq=>"Diego"}}]}
> User.where(query)
=> [#<User:0x00007fb70f371ec8 @doc={:_id=>"user/+04diego@gmail.com", :username=>"ErickFabian", :email=>"+04diego@gmail.com", :first_name=>"Diego", :last_name=>"Torres"}, @username="ErickFabian", @email="+04diego@gmail.com", @first_name="Diego", @last_name="Torres">]
> query = {and: [{ _id: { eq: 'user/+04diego@gmail.com' } } , { first_name: { eeq: 'Diego'}} ]}
=> {:and=>[{:_id=>{:eq=>"user/+04diego@gmail.com"}}, {:first_name=>{:eeq=>"Diego"}}]}
> User.where(query)
Traceback (most recent call last):
        1: from (irb):4
RuntimeError (invalid operator eeq)

@ErickFabian ErickFabian requested a review from javierg June 20, 2019 15:10
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

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

there are many features in this PR, can you remove anything not related to mango queries into new PRs please?

lib/dolly/callbacks.rb Outdated Show resolved Hide resolved
lib/dolly/callbacks.rb Outdated Show resolved Hide resolved
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

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

please add specs

lib/dolly/configuration.rb Outdated Show resolved Hide resolved
lib/dolly/mango.rb Outdated Show resolved Hide resolved
@brlanier
Copy link
Member

Did you see the work Sean had started here - #128

@javierg
Copy link
Member

javierg commented Jun 24, 2019

Did you see the work Sean had started here - #128

Ah, right... that was done for Dolly 2.x, 3.x is a different basecode, still @ErickFabian take a look and see if there is something that can help us.

@ErickFabian
Copy link
Contributor Author

@javierg Yeah i'll take a look at it, this seems like a different approach but i'll look into integrating it

@brlanier
Copy link
Member

@javierg Yeah i'll take a look at it, this seems like a different approach but i'll look into integrating it

I don't remember the details, but Sean worked hard to create some pattern or something that was going to make building queries easier. He was pretty excited about it at the time. I can't comment on whether it was good, bad or helpful. I just remember he put a fair amount of work into it, so it would be a shame to see it wasted.

lib/dolly/mango.rb Outdated Show resolved Hide resolved
lib/dolly/mango.rb Outdated Show resolved Hide resolved
lib/dolly/mango.rb Outdated Show resolved Hide resolved
end

def is_operator?(key)
COMBINATION_OPERATORS.include?(key) || CONDITION_OPERATORS.include?(key)
Copy link
Member

Choose a reason for hiding this comment

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

You can DRY this having a check on everything. Something like ALL_OPERATORS.include?(key)
Having a constant that include both ALL_OPERATORS = COMBINATION_OPERATORS + CONDITION_OPERATORS

@javierg
Copy link
Member

javierg commented Jun 25, 2019

I don't remember the details, but Sean worked hard to create some pattern or something that was going to make building queries easier. He was pretty excited about it at the time. I can't comment on whether it was good, bad or helpful. I just remember he put a fair amount of work into it, so it would be a shame to see it wasted.

Yes we will look at it, and see to integrate the query pattern, this PR is simply using the default JSON query lang from Mango, this will allow us to start working, later we can implement Seans API, unfortunately , Dolly 1 and 2 code has some fundamental issues with performance and architecture that makes that code incompatible with current Dolly 3

Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

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

:shipit:

@brlanier
Copy link
Member

I don't remember the details, but Sean worked hard to create some pattern or something that was going to make building queries easier. He was pretty excited about it at the time. I can't comment on whether it was good, bad or helpful. I just remember he put a fair amount of work into it, so it would be a shame to see it wasted.

Yes we will look at it, and see to integrate the query pattern, this PR is simply using the default JSON query lang from Mango, this will allow us to start working, later we can implement Seans API, unfortunately , Dolly 1 and 2 code has some fundamental issues with performance and architecture that makes that code incompatible with current Dolly 3

So Dolly 3 will be faster??? Sweet!!!

@ErickFabian ErickFabian merged commit b2f1e16 into 3.0 Jun 25, 2019
@ErickFabian ErickFabian deleted the feature/mango branch June 25, 2019 16:32
@javierg
Copy link
Member

javierg commented Jun 25, 2019

So Dolly 3 will be faster??? Sweet!!!

A little faster, but it retains a lot less objects in memory... and I mean A LOT LESS!

@brlanier
Copy link
Member

So Dolly 3 will be faster??? Sweet!!!

A little faster, but it retains a lot less objects in memory... and I mean A LOT LESS!

ohhhhhhh I will take that over the speed.... memory is always the issue in the apps and its usually because of large queries holding memory.

@seancookr
Copy link

👏 I miss couch

@brlanier
Copy link
Member

👏 I miss couch

It's here.... waiting for you....It misses you to @seancookr

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

Successfully merging this pull request may close these issues.

None yet

4 participants