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

SQL Comparison Operators #42

Closed
jwright opened this issue Jul 22, 2014 · 13 comments · Fixed by #46
Closed

SQL Comparison Operators #42

jwright opened this issue Jul 22, 2014 · 13 comments · Fixed by #46

Comments

@jwright
Copy link
Contributor

jwright commented Jul 22, 2014

I have the following method in a repository:

class WidgetRepository
  include Lotus::Repository

  def self.active
    query do
      where("publish_at >= #{Date.today}")
    end
  end
end

Calling the class method active via WidgetRepository.active results in the following error:

KeyError:
  key not found: "publish_at >= 2014-07-21"

I do not see any examples or documentation for this functionality within Lotus. Any help would be appreciated.

@carloslopes
Copy link

At this moment the .where method accept only a Hash. (https://github.com/lotus/model/blob/master/lib/lotus/model/adapters/sql/query.rb#L78-L117)

@jodosha do you have something in mind of how the API for these operator should be?

@runlevel5
Copy link
Member

@jodosha what do you think about adopting https://github.com/rails/arel?

@jodosha
Copy link
Member

jodosha commented Jul 28, 2014

@joneslee85 In which context? Repository, Adapter?

@ghost
Copy link

ghost commented Jul 28, 2014

@jodosha you can use sequel's where block condition. You can use it like this:

def self.where_age_greater_than(gt)
    query do
       where{ age > gt }
    end
end

I made an test implementation for this. coenert/model@6cdfd861a1ebe6783410690effb5faebc1dd3c51

@carloslopes
Copy link

@coenert nice! This way we don't need to use arel.

@runlevel5
Copy link
Member

@jodosha In Repository, I really like their powerful query builder.

@ghost
Copy link

ghost commented Jul 29, 2014

@joneslee85 ehm, i think the only way to adopt ARel is to use it as an sql query builder in our sql_adapter, ARel is just made to build queries without knowing the db. But sequel already provides most of the functions ARel has. @jodosha what's your opinion about using sequel's(or ARel's) api to build our queries(like my example)? The problem of this is that you need to wrap their documentation and you don't always know if there are little not documented features added that you don't want.

@jodosha
Copy link
Member

jodosha commented Jul 29, 2014

@coenert @carloslopes @joneslee85 Let's clarify one thing first: query API is not part of a repository. A repository is a client of that API, which is implemented by the adapters.

ARel: I really like it, but we can achieve the same results using Sequel for relational databases.

Now, I'm aware of the power of the lambda based API of Sequel. That wasn't introduced it in SqlAdapter::Query in the first release because I was worried of the overhead that the procs may introduce to resolve each single condition.

On the other hand we can have SQL literals, but they are dangerous for injections, and the author encourages to use of lambdas. Another con is couple repository implementation with specific SQL fragments, whereas lambdas are Ruby, so they are abstracted.

The way to follow are lambdas. @coenert Would you like to take care of this and create a PR? I noticed that you didn't tested the condition that triggers ArgumentError. Speaking of that, I ran some benchmarks. It turned out that the implementation that performs better on average is:

def where(condition = nil, &blk)
  condition or blk or raise ArgumentError.new('You need to specify an condition.')
  # ...
end

One unresolved issue is about database functions: how to express them with that API?
Sequel offers a solution:

dataset.update(:updated_at => Sequel.function(:NOW))
dataset.update(:updated_at => Sequel.lit('NOW()'))

But I see it cumbersome to use in situations like this: where('DATE(created_at) < NOW()').
What do you think about of the following proposal?

where{ function(:DATE, created_at) < function(:NOW) }

@ghost
Copy link

ghost commented Jul 29, 2014

@jodosha yes, i wil create a PR when i have time.
ArgumentError: nice!, i will implement it that way.
The problem with those functions is that you have to add functionality to Sequel's DSL, maybe we can build are own DSL for this wrapping sequel's dsl, and add functions to that dsl?

@carloslopes
Copy link

@jodosha I agree to use Sequel that is already a dependency to solve this is the better way.

@coenert solution is simple and effective.

👍

@ghost
Copy link

ghost commented Jul 29, 2014

@jodosha The only way to add the function method is adding an method to ::Sequel::SQL::VirtualRow.

class ::Sequel::SQL::VirtualRow
    def function(name, *args)
        return Sequel.function(name, *args)
    end
end

With this you can use function in you where queries:

where{ function(:DATE, Date.today) < function(:NOW) }

Do you agree with this implementation?

@jodosha
Copy link
Member

jodosha commented Jul 29, 2014

@coenert In my experience, reopening libraries is always source of maintenance pain.
Let's focus first on that lambda syntax, that will solve a lot of problems and we'll figure out how to support functions. Thanks for investigating and taking care of this. 👍

@ghost
Copy link

ghost commented Jul 29, 2014

@jodosha I can imagine your concerns. I think there are 2 ways of implementing functions in the lambda.

  • Subclassing VirtualRow, add functions and translate VirtualRow to sequel where conditions
  • Complete custom implementation of VirtualRow

For the long term i prefer option 2 because if you use Sequels implementation you are adding functions(and bugs) you don't know about, and because we always have the attributes to our disposal we don't have to implement method_missing.

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 a pull request may close this issue.

4 participants